[PATCH] D106313: Make more functions in MathExtras constexpr

[Duncan P. N. Exon Smith via llvm-commits]

Thanks for working on this!

One issue is that only system administrators are going to be allowed to leave reviews (Phabricator tells me "gAlfonso-bit created this object with edit policy "Administrators".") — you probably want to switch to the default policy.

Hopefully email replies still work!

I'm not sure I'm the right person to review the details of this, but a few other comments:
- There seem to be new `#error unsupported architecture` emitted in this patch — can/should those be landed first/separately? (Also, can you confirm whether there was UB previously in these cases?)
- Looks like you're adding an include of intrin.h here, and deleting a comment which explains why it wasn't previously included. Can you explain why it's okay now? (Might be good to add whoever added the comment as a reviewer in any case...)
- Can you confirm whether the functions you're modifying already have good functional coverage in unit tests? (Not everything does; since you're contributing new implementations seems like it's worth double-checking.)
- Not sure if we usually add explicit tests to confirm things are usable in a `constexpr` context, but if we do might be good to fix that here as well.
- When you update this / in future patches, please generate the patch with `-U99999999` or similar in order to provide full context to the Phabricator instance.
- I suggest running git-clang-format (should be available in your llvm-project checkout somewhere, but see https://clang.llvm.org/docs/ClangFormat.html for info) to canonicalize the whitespace on (just) the lines you're changing -- or you can manually apply the linter suggestions that clang-format sprinkled above if you like, but git-clang-format is easier.


> On 2021 Jul  19, at 14:26, Alf via Phabricator <reviews at reviews.llvm.org> wrote:
> 
> gAlfonso-bit created this revision.
> gAlfonso-bit created this object with edit policy "Administrators".
> gAlfonso-bit added a project: LLVM.
> Herald added a subscriber: dexonsmith.
> gAlfonso-bit requested review of this revision.
> Herald added a subscriber: llvm-commits.
> 
> A lot of these functions can be computed at compile time and would fare much better that way, especially since there are other functions that would benefit from being constexpr but call these functions and therefore do not get that luxury.
> 
> 
> Repository:
>  rG LLVM Github Monorepo
> 
> https://reviews.llvm.org/D106313
> 
> Files:
>  llvm/include/llvm/Support/MathExtras.h
> 
> <D106313.359914.patch>