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

Add VPCLMULQDQ crc32 tests to Google benchmarks #1651

Merged
merged 3 commits into from
Jan 27, 2024

Conversation

nmoinvaz
Copy link
Member

No description provided.

Copy link
Member

@Dead2 Dead2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Copy link

codecov bot commented Jan 25, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (1aa53f4) 83.13% compared to head (f49ddf8) 83.15%.

Files Patch % Lines
test/benchmarks/benchmark_crc32.cc 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1651      +/-   ##
===========================================
+ Coverage    83.13%   83.15%   +0.01%     
===========================================
  Files          135      135              
  Lines        10895    10898       +3     
  Branches      2816     2817       +1     
===========================================
+ Hits          9058     9062       +4     
  Misses        1127     1127              
+ Partials       710      709       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nmoinvaz
Copy link
Member Author

nmoinvaz commented Jan 25, 2024

Strange, it seems this potentially discovered a crash on GCC benchmark with VPCLMULQDQ. @KungFuJesus maybe you can take a look?

@Dead2
Copy link
Member

Dead2 commented Jan 25, 2024

Strange, it seems this potentially discovered a crash on GCC benchmark with VPCLMULQDQ.

The test machine doesn't have AVX512, so it shouldn't try to use VPCLMULQDQ at all. So looks like the cpu check is possibly at fault somehow.

@nmoinvaz
Copy link
Member Author

nmoinvaz commented Jan 25, 2024

Good observation I didn't think about that possibility.

@nmoinvaz nmoinvaz force-pushed the nathan/dev/benchmark-crc32 branch 2 times, most recently from cce0872 to a50fccc Compare January 25, 2024 23:32
@nmoinvaz
Copy link
Member Author

I think I missed some of the logic for the benchmark condition. I found existing logic elsewhere and just copied it.

@nmoinvaz
Copy link
Member Author

That doesn't seem to have fixed it. So maybe there is a bug in CPU runtime detection..

@nmoinvaz
Copy link
Member Author

nmoinvaz commented Jan 26, 2024

For example, here is what running the same crc32 tests on my desktop (without AVX512) using MSVC shows:
image

@nmoinvaz
Copy link
Member Author

Looks like that fixed it.

@nmoinvaz
Copy link
Member Author

nmoinvaz commented Jan 26, 2024

I'm not sure why the code coverage would be less...
image
Seems ok?

@mtl1979
Copy link
Collaborator

mtl1979 commented Jan 26, 2024

I'm not sure why the code coverage would be less... image Seems ok?

There is a lot of partial coverage on the lines with preprocessor macros... This suggests that some of the expanded code is not reached.

@nmoinvaz
Copy link
Member Author

Ah yea those Google testing/benchmark macros have a lot of boilerplate.

@Dead2 Dead2 merged commit 0dc9f73 into zlib-ng:develop Jan 27, 2024
138 of 139 checks passed
This was referenced May 30, 2024
@Dead2 Dead2 mentioned this pull request Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants