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

[Augie Fackler via Phabricator via llvm-commits]

durin42 marked an inline comment 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:
> 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.
Doc added, and follow-up for the bug that predates me in D119604 (I can reorder them if you'd like, I just don't really trust moz-phab to handle inserting a patch in the middle of the stack correctly...)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118263