Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changes to crc32 and st verify_benchmark routines #73

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

vb000
Copy link

@vb000 vb000 commented Jun 23, 2019

  • The crc32 result verified against (here) seems to be wrong. An x86 machine and a RISC-V machine appear to converge on 171698305.
  • If compiler optimization optimized this line, Coef = numerator / (sqrt(Aterm) * sqrt(Bterm));,
    into Coef = numerator / (sqrt(Aterm*Bterm));, the double FP result would be slightly different. To avoid failures due to these logically equivalent transformations, I added an epsilon check.

@bbbrumley
Copy link

I don't think that's the issue. I think the benchmark is returning 1 on success and 0 on failure, when it should be the opposite.

diff --git a/src/crc32/crc_32.c b/src/crc32/crc_32.c
index ad53ddd..a22f46f 100644
--- a/src/crc32/crc_32.c
+++ b/src/crc32/crc_32.c
@@ -202,9 +202,7 @@ int verify_benchmark(int r)
 {
   int expected = 1207487004;
 
-  if (r != expected)
-    return 0;
-  return 1;
+  return r != expected;
 }

@bbbrumley
Copy link

... or maybe I'm wrong and am misunderstanding the API.

But either way, I can verify both my x86_64 and armv7l targets did not like this test.

beebs/src/crc32$ ./crc32 
beebs/src/crc32$ echo $?
1

@bbbrumley
Copy link

Yea I was wrong -- the expected result is 0xfbda96cd. I checked it in a debugger, and also verified by dumping the binary inputs and checking with an external crc32 utility. So the fix is

diff --git a/src/crc32/crc_32.c b/src/crc32/crc_32.c
index ad53ddd..b65b31e 100644
--- a/src/crc32/crc_32.c
+++ b/src/crc32/crc_32.c
@@ -200,7 +200,7 @@ int benchmark()
 
 int verify_benchmark(int r)
 {
-  int expected = 1207487004;
+  int expected = -69560627;
 
   if (r != expected)
     return 0;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants