Skip to content
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.

Sign up for GitHub

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

Jump to bottom

<__msvc_int128.hpp>: Performance issues #2520

Open
2 of 4 tasks
CaseyCarter opened this issue Feb 4, 2022 · 7 comments
Open
2 of 4 tasks

<__msvc_int128.hpp>: Performance issues #2520

CaseyCarter opened this issue Feb 4, 2022 · 7 comments
Labels
performance Must go faster

Comments

@CaseyCarter
Copy link
Member

CaseyCarter commented Feb 4, 2022 •
edited

There are some potenial performance improvements to be made to the 128-bit integer-class types _Int128 and _Uint128 defined in <__msvc_int128.hpp>:

  • DevCom-1544675 results in terrible codegen when MSVC fails to fuse a division and modulo operation with equivalent operands into a single assembly instruction. Divides are expensive enough as it is, we don't want to pay double. The CRT's div and lldiv are also affected, so we'll need to use _udiv64/_div64 instead of raw / and % when !is_constant_evaluated(). The fix for this will ship "soon", it's not worth investing in a workaround.
  • Increment and decrement operations currently use a branch, but _AddCarry64 / _SubBorrow64 might be more efficient on x64 / arm64 (needs investigation).
  • _mm_and_si128 / _mm_or_si128 / _mm_xor_si128 could probably be used to implement bitops more efficiently at runtime on x64 (presumably arm64 has equivalents).
  • (Actually a throughput issue) __shiftleft128, _addcarry_u64, _subborrow_u64, and _udiv128 need to be moved from intrin.h to intrin0.h in vcruntime.

Thanks to @AlexGuteniev for suggesting all the above.
@CaseyCarter CaseyCarter added the performance Must go faster label Feb 4, 2022
@CaseyCarter CaseyCarter mentioned this issue Feb 4, 2022
Merged
6 tasks
@CaseyCarter CaseyCarter changed the title <__msvc_int128.hpp>: Workaround DevCom-1544675 <__msvc_int128.hpp>: Performance issues Feb 4, 2022
@AlexGuteniev
Copy link
Contributor

presumably by using div and lldiv

These are affected too. _div64 and _udiv64 intrinsic is the only viable workaround I'm aware of.

@CaseyCarter
Copy link
Member Author

  • (Actually a throughput issue) __shiftleft128, _addcarry_u64, _subborrow_u64, and _udiv128 need to be moved from intrin.h to intrin0.h in vcruntime.

I've moved these intrinsics in MSVC-PR-381950. We should be able to fixup the includes in __msvc_int128.hpp when that change ships in the first 17.3 preview with a toolset update (generally preview 1 or 2).

@fsb4000
Copy link
Contributor

fsb4000 commented Aug 11, 2022

DevCom-1544675 status: Fixed, Pending Release

@compnerd
Copy link

The inclusion of intrin.h which transitively includes intrin0.h causes some trouble for clang. In particular, the declaration of _subborrow_u64 and _addcarry_u64 result in conflicts. I've put up https://reviews.llvm.org/D139749 however, it is unclear whether this is reasonable as a workaround. Even with this workaround, include order will still cause an issue as the declaration following the inline definition seems to conflict.

@CaseyCarter CaseyCarter mentioned this issue Dec 13, 2022
Merged
@AlexGuteniev
Copy link
Contributor

DevCom-1544675 status: Fixed, Pending Release

DevCom-1544675 status fixed

@BurningEnlightenment
Copy link

Fixed in which version? No tag 'fixed in: xxx' has been added to the DevCom issue and there hasn't been a release today...

@CaseyCarter
Copy link
Member Author

CaseyCarter commented Feb 11, 2023 •
edited

Fixed in which version? No tag 'fixed in: xxx' has been added to the DevCom issue and there hasn't been a release today...

Looks like it was fixed for 17.4 preview 3, but the metadata wasn't set at the time, so the DevCom automation got confused.

compnerd added a commit to llvm/llvm-project that referenced this issue Apr 20, 2023
@compnerd
Headers: use C++ inline semantics in C++ mode
3b06779 
When building with the 17.5.0 preview toolset for MSVC and building with
modules, the definition of _addcarry_u64 and _subborrow_u64 seem to
cause issues due to the use of GNU inline semantics. Change the headers
to prefer C++ inline semantics for C++ compilation, falling back to GNU
inlining semantics for C compilation.

This is motivated by microsoft/STL#2520.

Differential Revision: https://reviews.llvm.org/D139749
Reviewed By: fsb4000
@compnerd compnerd mentioned this issue Apr 20, 2023
Merged
compnerd added a commit to apple/llvm-project that referenced this issue Apr 20, 2023
@compnerd
Headers: use C++ inline semantics in C++ mode
e291cd7 
When building with the 17.5.0 preview toolset for MSVC and building with
modules, the definition of _addcarry_u64 and _subborrow_u64 seem to
cause issues due to the use of GNU inline semantics. Change the headers
to prefer C++ inline semantics for C++ compilation, falling back to GNU
inlining semantics for C compilation.

This is motivated by microsoft/STL#2520.

Differential Revision: https://reviews.llvm.org/D139749
Reviewed By: fsb4000

(cherry picked from commit 3b06779)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
performance Must go faster
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@compnerd @CaseyCarter @AlexGuteniev @fsb4000 @BurningEnlightenment