Repository navigation
Use MSVC intrinsics for full multiplication - #5782
Conversation
🔴 Amalgamation check failed! 🔴The source code has not been amalgamated and/or formatted correctly, or 📎 A ready-to-apply patch is attached to the failed workflow run as the git apply amalgamation.patchThis does not require installing astyle yourself. @commit-msuyog Please read and follow the Contribution Guidelines. |
nlohmann
left a comment
There was a problem hiding this comment.
Thanks for the PR! The intrinsics are used correctly, and the branch order keeps clang-cl/icx on unsigned __int128. The msvc (x64) and msvc-arm64 jobs build with /W4 /WX and run ctest, so both new branches have been compiled and checked against the big-integer reference in CI.
A few things need to be done before this can be merged:
- DCO: The commit has no
Signed-off-byline. Please sign off your commits (git commit -s, orgit rebase --signoff developfor existing commits) and force-push. See the DCO check for details. - Formatting: The
checkjob fails because astyle formats theedge_casesinitializer intests/src/unit-class_lexer.cppdifferently. Runningmake amalgamatefixes it (it runs astyle as well). - Merge
develop: The other failures (-Wsuggest-attribute=purewith GCC,-Wexit-time-destructorsinunit-diagnostics-optimized.cppwith Clang, Infer, and the cancelled jobs) do not come from this PR. The branch is based on a commit before #5779, which fixed them. Mergingdevelopshould make CI green.
See the inline comments for smaller points.
On performance: as noted in #5780, the gain on develop is small because the only caller is the Eisel-Lemire fallback. The measurement on MSVC x64 (dump() of canada.json) makes more sense once the zero-copy view stack (#5768) has landed, so no benchmark is needed for this PR.
This review was written by Claude Code.
4e11ffe to
36c22ff
Compare
Signed-off-by: Suyog Verma <suyogverma0057@gmail.com>
Signed-off-by: Suyog Verma <suyogverma0057@gmail.com>
Signed-off-by: Suyog Verma <suyogverma0057@gmail.com>
36c22ff to
643ad52
Compare
|
Rebased onto the latest |
|
Thanks! Now let's wait for the Windows jobs and AppVeyor to pick this up. |
🔴 Amalgamation check failed! 🔴The source code has not been amalgamated and/or formatted correctly, or 📎 A ready-to-apply patch is attached to the failed workflow run as the git apply amalgamation.patchThis does not require installing astyle yourself. @commit-msuyog Please read and follow the Contribution Guidelines. |
1 similar comment
🔴 Amalgamation check failed! 🔴The source code has not been amalgamated and/or formatted correctly, or 📎 A ready-to-apply patch is attached to the failed workflow run as the git apply amalgamation.patchThis does not require installing astyle yourself. @commit-msuyog Please read and follow the Contribution Guidelines. |
full_multiplication()currently usesunsigned __int128on GCC/Clang and falls back to four 32-bit partial products everywhere else. This adds the native intrinsics for the two MSVC targets that have them._umul128(a, b, &high)gives both halves of the 128-bit product.a * bprovides the low half and__umulh(a, b)provides the high half.<intrin.h>is included only for MSVC x64/ARM64 builds, so other compilers and platforms are unaffected.__SIZEOF_INT128__branch remains first. This is important for clang-cl, which defines that macro and should continue usingunsigned __int128.I extended the existing "128-bit products and leading zeros" test section with fixed edge cases checked against the existing big-integer reference, including
0,1,UINT64_MAX, values around2^32, and large boundary values. The existing deterministic random-pair coverage is also preserved.single_include/nlohmann/json.hppis synchronized with the implementation change.Testing: the touched tests pass locally with ASan and UBSan, and
git diff --checkis clean. MSVC/clang-cl are not available in my local environment, so the new MSVC branches have not been compiled locally. The repository's Windows CI jobs should provide compiler-specific validation.Performance: I have not benchmarked this change. The goal is to use the native 64×64-bit multiplication support provided by MSVC rather than the portable partial-product implementation.
Closes #5780