-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
Suggestion about AcceleratedCRC32C() #488
Conversation
…leratedCRC32C() in file port_posix_sse.cc
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
I signed it! |
CLAs look good, thanks! |
Can you please submit a test that the original code fails, but your test passes? You can use the non-accelerated implementation to get a reference value. |
I know the original code can get the right result. However, the original code can’t do a right alignment work. |
@VincentVessalius Thank you for explaining! The explanation makes sense, and a fix is on the way. |
When faced with a pointer that is misaligned by K bytes (pointer % 8 == K), the code previously moved forward by K bytes. In order to end up with an aligned pointer, the code must move by 8 - K bytes. This lands google#488 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=166295921
When faced with a pointer that is misaligned by K bytes (pointer % 8 == K), the code previously moved forward by K bytes. In order to end up with an aligned pointer, the code must move by 8 - K bytes. This lands google#488 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=166295921
This landed in 2964b80. Thank you very much for your contribution! |
…e#488) As @dominichamon and I have discussed, the current reporter interface is poor at best. And something should be done to fix it. I strongly suspect such a fix will require an entire reimagining of the API, and therefore breaking backwards compatibility fully. For that reason we should start deprecating and removing parts that we don't intend to replace. One of these parts, I argue, is the CSVReporter. I propose that the new reporter interface should choose a single output format (JSON) and traffic entirely in that. If somebody really wanted to replace the functionality of the CSVReporter they would do so as an external tool which transforms the JSON. For these reasons I propose deprecating the CSVReporter.
I think the Process(unaligned bytes) in function in file port_posix_sse.cc maybe wrong.
The original code does m times STEP1, where m = p%8.
If p=10,then m=2,and the new p will be equal to 12, which is not aligned by 8.
Thannks for reading.