getAllocAlignment: respect allocalign attribute if present

[durin42]

Summary

As with allocsize(), we prefer the table data to attributes.

[reames]

Patch appears to be stale, general direction looks reasonable. Marking as require changes to indicate author action needed once dependency lands.

[jyknight]

llvm/lib/Analysis/MemoryBuiltins.cpp

332

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.

llvm/lib/Transforms/Utils/BuildLibCalls.cpp

228 ↗

(On Diff #406915)

(Ah, I see, this fix landed in the wrong diff in the stack -- should've been in previous).

[durin42]

llvm/lib/Analysis/MemoryBuiltins.cpp

332

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

[jyknight]

llvm/lib/Analysis/MemoryBuiltins.cpp

332

"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.

[durin42]

llvm/lib/Analysis/MemoryBuiltins.cpp

332

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

[nikic]

llvm/lib/Analysis/MemoryBuiltins.cpp

339

Stray semicolon

342

You should use hasAttrSomewhere here (it's more efficient if the attribute is not present anywhere).

llvm/include/llvm/Analysis/MemoryBuiltins.h

 /// Ex: If our allocation routine has a counter for the number of objects
 /// allocated, and the program prints it on exit, can the value change due
 /// to optimization? Answer is highly language dependent.
 /// Note: *Removable* really does mean removable; it does not mean observable.
 /// A language (e.g. C++) can allow removing allocations without allowing
 /// insertion or speculative execution of allocation routines.
 bool isAllocRemovable(const CallBase *V, const TargetLibraryInfo *TLI);
 
-/// Gets the alignment argument for an aligned_alloc-like function
+/// Gets the alignment argument for an aligned_alloc-like function, using either
+/// built-in knowledge based on fuction names/signatures or allocalign
+/// attributes. Note: the Value returned may not indicate a valid alignment, per
+/// the definition of the allocalign attribute.
 Value *getAllocAlignment(const CallBase *V, const TargetLibraryInfo *TLI);
 
 /// Return the size of the requested allocation.  With a trivial mapper, this is
 /// identical to calling getObjectSize(..., Exact).  A mapper function can be
 /// used to replace one Value* (operand to the allocation) with another.  This
 /// is useful when doing abstract interpretation.
 Optional<APInt> getAllocSize(const CallBase *CB,
                              const TargetLibraryInfo *TLI,

llvm/lib/Analysis/MemoryBuiltins.cpp

bool llvm::isAllocRemovable(const CallBase *CB, const TargetLibraryInfo *TLI) {
   // example, recent C++ requires direct calls to the global allocation
   // [basic.stc.dynamic.allocation] to be observable unless part of a new
   // expression [expr.new paragraph 13].
 
   // Historically we've treated the C family allocation routines as removable
   return isAllocLikeFn(CB, TLI);
 }
 
 Value *llvm::getAllocAlignment(const CallBase *V,
                                const TargetLibraryInfo *TLI) {
   assert(isAllocationFn(V, TLI));
 
   const Optional<AllocFnsTy> FnData = getAllocationData(V, AnyAlloc, TLI);
-  if (!FnData.hasValue() || FnData->AlignParam < 0) {
-    return nullptr;
-  }
+  if (FnData.hasValue() && FnData->AlignParam >= 0) {
     return V->getOperand(FnData->AlignParam);
   }
+  unsigned AllocAlignParam;
+  if (V->getAttributes().hasAttrSomewhere(Attribute::AllocAlign,
+                                          &AllocAlignParam)) {
+    return V->getOperand(AllocAlignParam-1);
+  }
+  return nullptr;
+}
 
 /// When we're compiling N-bit code, and the user uses parameters that are
 /// greater than N bits (e.g. uint64_t on a 32-bit build), we can run into
 /// trouble with APInt size issues. This function handles resizing + overflow
 /// checks for us. Check and zext or trunc \p I depending on IntTyBits and
 /// I's value.
 static bool CheckedZextOrTrunc(APInt &I, unsigned IntTyBits) {
   // More bits than we can handle. Checking the bit width isn't necessary, but