[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) {
----------------
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.


================
Comment at: llvm/lib/Transforms/Utils/BuildLibCalls.cpp:228
 static bool setAlignedAllocParam(Function &F, unsigned ArgNo) {
-  if (F.hasFnAttribute(Attribute::AllocAlign)) {
+  if (F.hasParamAttribute(ArgNo, Attribute::AllocAlign)) {
     return false;
----------------
(Ah, I see, this fix landed in the wrong diff in the stack -- should've been in previous).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118263