This is an archive of the discontinued LLVM Phabricator instance.

[LLVM][Support][NFC] Make multiple bitwise MathExtras functions constexpr
AbandonedPublic

Authored by gAlfonso-bit on Aug 1 2021, 11:35 AM.

Details

Reviewers
dexonsmith
MaskRay
Group Reviewers
Restricted Project
Summary

Submitted as an upstream fix

MathExtras has many useful tools. However, they aren't available to use in constexpr equations or functions. Originally this was a patch for swift llvm but it works upstream and as such I have put this PR upstream.

Diff Detail

Event Timeline

gAlfonso-bit created this revision.Aug 1 2021, 11:35 AM
gAlfonso-bit requested review of this revision.Aug 1 2021, 11:35 AM
gAlfonso-bit edited the summary of this revision. (Show Details)Aug 2 2021, 8:34 AM
gAlfonso-bit retitled this revision from Make multiple mathextras functions constexpr to [LLVM]ake multiple mathextras functions constexpr.
gAlfonso-bit retitled this revision from [LLVM]ake multiple mathextras functions constexpr to [LLVM] Make multiple mathextras functions constexpr.Aug 2 2021, 9:48 AM
gAlfonso-bit retitled this revision from [LLVM] Make multiple mathextras functions constexpr to [LLVM][NFC] Make multiple mathextras functions constexpr.Aug 2 2021, 9:55 AM
gAlfonso-bit retitled this revision from [LLVM][NFC] Make multiple mathextras functions constexpr to [LLVM][NFC] Make multiple bitwise MathExtras functions constexpr.Aug 2 2021, 11:30 AM
gAlfonso-bit retitled this revision from [LLVM][NFC] Make multiple bitwise MathExtras functions constexpr to [LLVM][Support][NFC] Make multiple bitwise MathExtras functions constexpr.Aug 4 2021, 8:17 AM
gAlfonso-bit retitled this revision from [LLVM][Support][NFC] Make multiple bitwise MathExtras functions constexpr to [LLVM][NFC] Make multiple bitwise MathExtras functions constexpr.Aug 4 2021, 8:21 AM
gAlfonso-bit edited reviewers, added: MaskRay; removed: sivachandra.Aug 4 2021, 10:32 AM
gAlfonso-bit edited the summary of this revision. (Show Details)Aug 4 2021, 12:09 PM

Made even more functions constexpr

Fix formatting

Fix compilation errors

Please upload patches with full context using -U999999 per the instructions here https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

Please upload patches with full context using -U999999 per the instructions here https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

Alright, I did.

craig.topper added inline comments.Aug 4 2021, 4:03 PM
llvm/include/llvm/Support/MathExtras.h
212

Does _BitScanReverse work in a constexpr in MSVC? The implementation in clang-cl doesn't work in constexpr, but clang-cl would use __builtin_clz.

gAlfonso-bit marked an inline comment as done.Aug 4 2021, 4:15 PM
gAlfonso-bit added inline comments.
llvm/include/llvm/Support/MathExtras.h
212

Yes it does. This is one of those exceptions where you can pass a reference because you don't read Index, but only write to it, and Index is in a constexpr anyway

craig.topper added inline comments.Aug 4 2021, 4:21 PM
llvm/include/llvm/Support/MathExtras.h
212

I tried it on godbolt and it didn't seem to work. Am I doing something wrong? https://godbolt.org/z/MnEY763E4

gAlfonso-bit marked 2 inline comments as done.Aug 4 2021, 4:27 PM
gAlfonso-bit added inline comments.
llvm/include/llvm/Support/MathExtras.h
212

Not sure, but so far LLVM compiles on Windows without a hitch, and the testbot is not having issues either

gAlfonso-bit marked an inline comment as done.

clang-tidy is complaining about function names, but changing every function instance would modify too many files

craig.topper added inline comments.Aug 4 2021, 5:04 PM
llvm/include/llvm/Support/MathExtras.h
212

Maybe that just means llvm never uses it in a context where it needs to be evaluated as a constexpr since your patch didn't add any new usages. So it might fail if someone tries to use the constexpr capability?

gAlfonso-bit added inline comments.Aug 4 2021, 5:12 PM
llvm/include/llvm/Support/MathExtras.h
212

Well, the other functions I made constexpr calls them, so that isn't it.

gAlfonso-bit marked an inline comment as done.Aug 4 2021, 5:16 PM

@craig.topper Only clang-tidy is having issues

I copied a chunk of this patch to godbolt and added two different constexpr usages of countLeadingZeros that both fail with MSVC https://godbolt.org/z/Maen4bac4 I'm guessing that because everything is a template, MSVC doesn't diagnose anything until you try to use it in constexpr context.

RKSimon added a subscriber: RKSimon.Aug 5 2021, 2:06 AM
craig.topper added inline comments.Aug 5 2021, 9:07 AM
llvm/include/llvm/Support/MathExtras.h
113–118

Doesn't this mean that users of this that want to use it in constexpr can't compile that code with MSVC and would need a different solution just for MSVC? Is that useful for the project?

gAlfonso-bit added inline comments.Aug 5 2021, 10:45 AM
llvm/include/llvm/Support/MathExtras.h
113–118

No, since people who want to use it with constexpr but can't already exist, but this group gets smaller. Hopefully Microsoft finds a way, but there are downstream projects that would benefit from this already, like Swift.

By the way, it is not windows that gives clang tidy errors, but Linux.

gAlfonso-bit marked an inline comment as done.EditedAug 5 2021, 11:17 AM

Unfortunately it can't be constexpr for Windows at this time.

gAlfonso-bit retitled this revision from [LLVM][NFC] Make multiple bitwise MathExtras functions constexpr to [LLVM][Support][NFC] Make multiple bitwise MathExtras functions constexpr.Aug 5 2021, 11:30 AM

No build errors, only clang-tidy ones!

Remove contexpr from functions that call _BitScan Microsoft functions

This comment was removed by gAlfonso-bit.
llvm/include/llvm/Support/MathExtras.h
212

So I think I figured it out. Since I made the FIRST count function constexpr where the second value in the <> braces isn't constant, the compiler is using that instead of the non constexpr ones in these cases.

Is it possible to add unittests showing the count functions being used in constexpr contexts to prove that this will work with MSVC?

Unfortunately, as long as we are using c++14, there is actually no easy solution to this

gAlfonso-bit marked an inline comment as not done.Aug 8 2021, 8:41 AM

@craig.topper I could use the libcpp_clz, which is constexpr, but that seems to be c++ 20 only.

gAlfonso-bit abandoned this revision.Aug 11 2021, 10:55 AM

I don't see how to progress here, so I am closing this for now