Fix use of crc32c 3way on portable builds using MSVC #10667
Conversation
|
Hi @danielen1337! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
danielen1337
force-pushed
the
fix_crc32_3way_on_portable_builds_using_msvc
branch
from
38baa04
to
c9b47ba
Compare
|
@ajkr @pdillinger Do you have any feedback on this? |
|
@adamretter are you be able to help take a look? |
|
Hi @siying @adamretter has asked me to review this on his behalf. Could you approve the workflows so we can sanity check that the builds (MSVC and the rest) are OK ? On initial inspection the code looks to me as if it does as intended. |
|
Hi @danielen1337 - thinking about tests. This looks like something that would be tricky to test in a CI way; would come down to timing a test and needing specific hardware. I take it you've confirmed for your MSVC platform(s) that with this PR crc32c is now used when you have PORTABLE and MSVC ? |
|
Hi @danielen1337; see above failed check. You need to "$ make format" on your branch to ensure that it conforms to the formatting requirements. |
danielen1337
force-pushed
the
fix_crc32_3way_on_portable_builds_using_msvc
branch
from
c9b47ba
to
88878eb
Compare
danielen1337
force-pushed
the
fix_crc32_3way_on_portable_builds_using_msvc
branch
from
88878eb
to
571fd3f
Compare
|
Hi @siying @kobykahane @adamretter this LGTM, format has been fixed, but needs someone to approve workflows on the contributor so that the CI can run. |
|
Thanks @alanpaxton , I've check the use-case of portable build using MSVC and it uses the optomized version of the crc as expected. |
|
@ajkr has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@danielen1337 has updated the pull request. You must reimport the pull request before landing. |
|
@ajkr has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@danielen1337 has updated the pull request. You must reimport the pull request before landing. |
|
@ajkr When can this be merged? Thank you |
|
@ajkr has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
It needs a second employee to accept the internal diff. I will try to make sure that happens in time for the version 7.9 branch cut, which is in 2.5 weeks. Usually this process would finish much sooner than that though. |
Hello,
As discussed previously in this discussion, the mentioned PR introduced a regression in portable versions that compile with MSVC - crc_3way optimization won't be used even in cases where it is supported.
This PR aims to fix just that.