<__msvc_int128.hpp>: optimize _Signed128::operator* with a branchless multiply - #6406
<__msvc_int128.hpp>: optimize _Signed128::operator* with a branchless multiply#6406KKoishi_ (Koishi-Satori) wants to merge 1 commit into
<__msvc_int128.hpp>: optimize _Signed128::operator* with a branchless multiply#6406Conversation
…ly without sign-normalizes
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Optimizes _Signed128 multiplication by avoiding sign normalization and adds a microbenchmark to compare new vs. old behavior/performance.
Changes:
- Replaced
_Signed128::operator*implementation with a direct low-128-bit product using_UMul128+ cross terms. - Added a new Google Benchmark program to compare the new multiplication vs. the previous implementation.
- Registered the new benchmark target in the benchmarks CMake list.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| stl/inc/__msvc_int128.hpp | Reworks _Signed128 multiplication implementation to compute the low 128 bits directly. |
| benchmarks/src/signed128_mul.cpp | Adds a benchmark comparing new vs. old signed-128 multiplication. |
| benchmarks/CMakeLists.txt | Adds the new signed128_mul benchmark target. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // The low 128 bits of a two's-complement product equal the unsigned product, so the sign normalization in the old | ||
| // implementation was unnecessary. | ||
| _Signed128 _Result; | ||
| _Result._Word[0] = _UMul128(_Left._Word[0], _Right._Word[0], _Result._Word[1]); |
There was a problem hiding this comment.
This is wrong. _UMul128 is unconditionally constexpr.
| _Signed128 _Result; | ||
| _Result._Word[0] = _UMul128(_Left._Word[0], _Right._Word[0], _Result._Word[1]); |
There was a problem hiding this comment.
This is wrong. _UMul128 is unconditionally provided.
There was a problem hiding this comment.
This is wrong.
_UMul128is unconditionally provided.
Thanks, it seems like _UMul128 has been unconditionally available since #6356
| #include <array> | ||
| #include <cstdint> | ||
| #include <random> | ||
| #include <utility> |
There was a problem hiding this comment.
I'll also suggest including <vector>. But one should be also aware that the name _Signed128 is MSVC STL-specific if the name is already in the sight.
| vector<pair<_Signed128, _Signed128>> init_test_set() { | ||
| vector<pair<_Signed128, _Signed128>> vec(1'000'000); |
| bool _negative = false; | ||
| _Left._Strip_negative(_negative); | ||
| _Right._Strip_negative(_negative); |
|
@microsoft-github-policy-service agree |
| _Result = -_Result; | ||
| } | ||
| // The low 128 bits of a two's-complement product equal the unsigned product, so the sign normalization in the old | ||
| // implementation was unnecessary. |
There was a problem hiding this comment.
I'm not a huge fan of comments about old versions of the code. That's what commit messages and git blame is for.
Resolves issue #6405.
std::_Signed128::operator*in<__msvc_int128.hpp>perform sign-normalizes before multiplying, but it is unnecessary and will cause performance issue.The low 128 bits of a two's-complement product equal the unsigned product, so the sign handling is unnecessary. I replaced it with the same computation
_Base128::_Multiplyalready performs.I've tried to implement a version without sign handling, the new version has same behavior and will not break any ABI. The new version without sign-normalizing is more faster than current version in
<__msvc_int128.hpp>.This PR also adds a signed128_mul benchmark comparing the new implementation against a faithful replica of the old one.