[PATCH] D118263: getAllocAlignment: respect allocalign attribute if present

[Augie Fackler via Phabricator via llvm-commits]

durin42 marked 2 inline comments as done.
durin42 added inline comments.


================
Comment at: llvm/lib/Analysis/MemoryBuiltins.cpp:333
 
 Value *llvm::getAllocAlignment(const CallBase *V,
                                const TargetLibraryInfo *TLI) {
----------------
jyknight wrote:
> Need to update the docs for the function to mention that it uses the alloc_align attribute. Also mention that it can return invalid alignments and callers need to be able to handle that.
> 
> And, then, need to actually verify the existing callers of this function can accept an invalid alignment value, since now we're providing one. But in fact, they cannot (as per previous discussion). 
> 
> That is, llvm/lib/Transforms/IPO/AttributorAttributes.cpp will try to make an alloca instruction with an invalid alignment, which is not permitted.
> 
> Basically, the code:
> ```
>     if (Value *Align = getAllocAlignment(AI.CB, TLI)) {
>       if (!getAPInt(A, *this, *Align)) {
>         [[error case]]
> ```
> needs to also verify against MaximumAlignment and power-of-2ness, just like the other callsite, in llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp already does.
> 
> Probably good to add a helper function, for getting a constant alignment value, e.g. as a `Optional<uint64_t>`, which calls this function then verifies the result is valid before returning.
Updated the docs that it uses allocalign, but I'm not clear what wording I should be putting on the function about invalid alignment values: this function doesn't return the alignment, but the argument used for alignment. AttributorAttributes.cpp was broken before my change was written!

Can you elaborate on what warning label you want on this function WRT the validation callers should do? I can add it as a later patch as a cleanup while I'm here (but I'm not going to go fix AttributorAttributes right now since it's been broken all along...)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118263