[PATCH] D68141: [Alignment][NFC] Remove AllocaInst::setAlignment(unsigned)

Wed Oct 2 04:30:22 PDT 2019

gchatelet marked an inline comment as done.
gchatelet added inline comments.

================
Comment at: llvm/trunk/lib/Target/AArch64/AArch64StackTagging.cpp:65

-static constexpr unsigned kTagGranuleSize = 16;
+static const Align kTagGranuleSize = Align(16);

----------------
arichardson wrote:
> gchatelet wrote:
> > arichardson wrote:
> > > Can't the Align ctor be constexpr? Will this result in a `Log2` call at runtime?
> > The implementation of Log2 is quite complex and depends on the implementation of [[ https://github.com/llvm/llvm-project/blob/02ada9bd2b41d850876a483bede59715e7550c1e/llvm/include/llvm/Support/MathExtras.h#L127 | countLeadingZeros ]].
> > GCC, clang, ICC can compute the value at compile time but there may be some compiler that can't.
> > https://godbolt.org/z/ytwWHn
> > 
> > I could solve this issue by introducing a `LogAlign()` function returning an `Align`. This function could be `constexpr`. What do you think? Is it worth the additional complexity?
> Since there won't a global ctor at -O2 or higher it probably doesn't matter.
> 
> I'm not sure if there have been any of these cases yet, but I would think that having a `LogAlign/Align::fromLog()/etc.` might still be useful for some cases where you get the alignment as log2 instead of in bytes?
> This would avoid additional work for the compiler and might be easier to read since you can omit the `1<<N`.
I'll wait a bit for the `LogAlign` to check if it's useful.
Meanwhile you may want to have a look at D68329 where I allow `constexpr` `Align`.

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68141/new/

https://reviews.llvm.org/D68141