Skip to content

Use MSVC intrinsics for full multiplication - #5782

Merged
nlohmann merged 3 commits into
nlohmann:developfrom
commit-msuyog:fix/msvc-full-multiplication
Oct 8, 2026
Merged

nlohmann merged 3 commits into
nlohmann:developfrom
commit-msuyog:fix/msvc-full-multiplication

Conversation

@commit-msuyog

Copy link
Copy Markdown
Contributor

full_multiplication() currently uses unsigned __int128 on 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.

  • x64: _umul128(a, b, &high) gives both halves of the 128-bit product.
  • ARM64: a * b provides 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.
  • The __SIZEOF_INT128__ branch remains first. This is important for clang-cl, which defines that macro and should continue using unsigned __int128.
  • 32-bit MSVC and other compilers continue to use the existing portable fallback unchanged.

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 around 2^32, and large boundary values. The existing deterministic random-pair coverage is also preserved.

single_include/nlohmann/json.hpp is synchronized with the implementation change.

Testing: the touched tests pass locally with ASan and UBSan, and git diff --check is 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

@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown

🔴 Amalgamation check failed! 🔴

The source code has not been amalgamated and/or formatted correctly, or BUILD.bazel is out of date.

📎 A ready-to-apply patch is attached to the failed workflow run as the amalgamation-patch artifact. Download it, then apply it locally from the repository root with:

git apply amalgamation.patch

This does not require installing astyle yourself.

@commit-msuyog Please read and follow the Contribution Guidelines.

@nlohmann nlohmann added the please rebase Please rebase your branch to origin/develop label Oct 7, 2026
@nlohmann

nlohmann commented Oct 7, 2026

Copy link
Copy Markdown
Owner

Please rebase to the latest develop: CI on develop is green again since #5779, so a rebase should give this PR a clean CI run.

(Comment written by Claude Code on behalf of @nlohmann.)

@nlohmann nlohmann left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

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:

  1. DCO: The commit has no Signed-off-by line. Please sign off your commits (git commit -s, or git rebase --signoff develop for existing commits) and force-push. See the DCO check for details.
  2. Formatting: The check job fails because astyle formats the edge_cases initializer in tests/src/unit-class_lexer.cpp differently. Running make amalgamate fixes it (it runs astyle as well).
  3. Merge develop: The other failures (-Wsuggest-attribute=pure with GCC, -Wexit-time-destructors in unit-diagnostics-optimized.cpp with Clang, Infer, and the cancelled jobs) do not come from this PR. The branch is based on a commit before #5779, which fixed them. Merging develop should 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.

Comment thread include/nlohmann/detail/bit_ops.hpp Outdated
Comment thread tests/src/unit-class_lexer.cpp Outdated
Comment thread tests/src/unit-class_lexer.cpp Outdated
@commit-msuyog
commit-msuyog force-pushed the fix/msvc-full-multiplication branch from 4e11ffe to 36c22ff Compare October 7, 2026 15:43
@nlohmann

nlohmann commented Oct 7, 2026

Copy link
Copy Markdown
Owner

The branch is still based on 30e542e, so the GCC, Clang and Infer failures already fixed on develop by #5779 will probably come back. Those failures aren't caused by this PR. The new commits only touch bit_ops.hpp and the test, which don't affect them.

Signed-off-by: Suyog Verma <suyogverma0057@gmail.com>
Signed-off-by: Suyog Verma <suyogverma0057@gmail.com>
Signed-off-by: Suyog Verma <suyogverma0057@gmail.com>
@commit-msuyog
commit-msuyog force-pushed the fix/msvc-full-multiplication branch from 36c22ff to 643ad52 Compare October 7, 2026 16:10
@commit-msuyog

Copy link
Copy Markdown
Contributor Author

Rebased onto the latest develop including #5779.

@nlohmann nlohmann removed the please rebase Please rebase your branch to origin/develop label Oct 7, 2026

@nlohmann nlohmann left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Looks good to me.

@nlohmann nlohmann added the 🚀 ready to merge Ready to merge - just waiting for CI to complete. label Oct 7, 2026
@nlohmann nlohmann added this to the Release 3.13.0 milestone Oct 7, 2026
@nlohmann

nlohmann commented Oct 7, 2026

Copy link
Copy Markdown
Owner

Thanks! Now let's wait for the Windows jobs and AppVeyor to pick this up.

@nlohmann
nlohmann merged commit a269794 into nlohmann:develop Oct 8, 2026
170 of 172 checks passed
@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown

🔴 Amalgamation check failed! 🔴

The source code has not been amalgamated and/or formatted correctly, or BUILD.bazel is out of date.

📎 A ready-to-apply patch is attached to the failed workflow run as the amalgamation-patch artifact. Download it, then apply it locally from the repository root with:

git apply amalgamation.patch

This does not require installing astyle yourself.

@commit-msuyog Please read and follow the Contribution Guidelines.

1 similar comment
@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown

🔴 Amalgamation check failed! 🔴

The source code has not been amalgamated and/or formatted correctly, or BUILD.bazel is out of date.

📎 A ready-to-apply patch is attached to the failed workflow run as the amalgamation-patch artifact. Download it, then apply it locally from the repository root with:

git apply amalgamation.patch

This does not require installing astyle yourself.

@commit-msuyog Please read and follow the Contribution Guidelines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

M 🚀 ready to merge Ready to merge - just waiting for CI to complete. tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use MSVC intrinsics for the 64×64-bit multiplication in full_multiplication()

2 participants