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

[James Y Knight via Phabricator via llvm-commits]

jyknight added inline comments.


================
Comment at: llvm/lib/Analysis/MemoryBuiltins.cpp:333
 
 Value *llvm::getAllocAlignment(const CallBase *V,
                                const TargetLibraryInfo *TLI) {
----------------
durin42 wrote:
> 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...)
"Note: the Value returned may not indicate a valid alignment, per the definition of the allocalign attribute."

I do think it's on you to fix this, despite it being broken before for certain hardcoded known functions, because _now_ you've defined a brand-new attribute which supposedly has certain semantics, yet the code doesn't honor those semantics. That's not really OK, regardless of the state of things for the hardcoded alloc_aligned/memalign functions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118263