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

333

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

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

[durin42]

llvm/lib/Analysis/MemoryBuiltins.cpp

333

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

333

"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

333

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

340

Stray semicolon

343

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

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);
+    ;
+  }
+  for (unsigned i = 0; i < V->arg_size(); ++i) {
+    if (V->paramHasAttr(i, Attribute::AllocSize)) {
+      return V->getOperand(i);
+    }
+  }
+  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) {

llvm/lib/Transforms/Utils/BuildLibCalls.cpp

static bool setWillReturn(Function &F) {
   if (F.hasFnAttribute(Attribute::WillReturn))
     return false;
   F.addFnAttr(Attribute::WillReturn);
   ++NumWillReturn;
   return true;
 }
 
 static bool setAlignedAllocParam(Function &F, unsigned ArgNo) {
-  if (F.hasFnAttribute(Attribute::AllocAlign)) {
+  if (F.hasParamAttribute(ArgNo, Attribute::AllocAlign)) {
     return false;
   }
-  F.addFnAttr(Attribute::get(F.getContext(), Attribute::AllocAlign,
-                             static_cast<uint64_t>(ArgNo + 1)));
+  F.addParamAttr(ArgNo, Attribute::AllocAlign);
   return true;
 }
 
 bool llvm::inferLibFuncAttributes(Module *M, StringRef Name,
                                   const TargetLibraryInfo &TLI) {
   Function *F = M->getFunction(Name);
   if (!F)
     return false;

llvm/test/Transforms/InferFunctionAttrs/annotate.ll

 declare float @acoshf(float)
 
 ; CHECK: declare x86_fp80 @acoshl(x86_fp80) [[NOFREE_NOUNWIND_WILLRETURN_WRITEONLY]]
 declare x86_fp80 @acoshl(x86_fp80)
 
 ; CHECK: declare x86_fp80 @acosl(x86_fp80) [[NOFREE_NOUNWIND_WILLRETURN_WRITEONLY]]
 declare x86_fp80 @acosl(x86_fp80)
 
-; CHECK: declare noalias noundef i8* @aligned_alloc(i64 noundef, i64 noundef) [[INACCESSIBLEMEMONLY_NOFREE_NOUNWIND_WILLRETURN_ALIGN:#[0-9]+]]
+; CHECK: declare noalias noundef i8* @aligned_alloc(i64 allocalign noundef, i64 noundef) [[INACCESSIBLEMEMONLY_NOFREE_NOUNWIND_WILLRETURN:#[0-9]+]]
 declare i8* @aligned_alloc(i64, i64)
 
 ; CHECK: declare double @asin(double) [[NOFREE_NOUNWIND_WILLRETURN_WRITEONLY]]
 declare double @asin(double)
 
 ; CHECK: declare float @asinf(float) [[NOFREE_NOUNWIND_WILLRETURN_WRITEONLY]]
 declare float @asinf(float)
 

 declare i32 @lstat(i8*, %opaque*)
 
 ; CHECK-LINUX: declare noundef i32 @lstat64(i8* nocapture noundef readonly, %opaque* nocapture noundef) [[NOFREE_NOUNWIND]]
 declare i32 @lstat64(i8*, %opaque*)
 
 ; CHECK: declare noalias noundef i8* @malloc(i64 noundef) [[INACCESSIBLEMEMONLY_NOFREE_NOUNWIND_WILLRETURN]]
 declare i8* @malloc(i64)
 
-; CHECK-LINUX: declare noalias noundef i8* @memalign(i64, i64) [[INACCESSIBLEMEMONLY_NOFREE_NOUNWIND_WILLRETURN_ALIGN:#[0-9]+]]
+; CHECK-LINUX: declare noalias noundef i8* @memalign(i64 allocalign, i64) [[INACCESSIBLEMEMONLY_NOFREE_NOUNWIND_WILLRETURN:#[0-9]+]]
 declare i8* @memalign(i64, i64)
 
 ; CHECK: declare i8* @memccpy(i8* noalias writeonly, i8* noalias nocapture readonly, i32, i64) [[ARGMEMONLY_NOFREE_NOUNWIND_WILLRETURN]]
 declare i8* @memccpy(i8*, i8*, i32, i64)
 
 ; CHECK-LINUX:   declare i8* @memchr(i8*, i32, i64) [[ARGMEMONLY_NOFREE_NOUNWIND_READONLY]]
 ; CHECK-DARWIN:  declare i8* @memchr(i8*, i32, i64) [[ARGMEMONLY_NOFREE_NOUNWIND_READONLY:#[0-9]+]]
 ; CHECK-UNKNOWN: declare i8* @memchr(i8*, i32, i64) [[ARGMEMONLY_NOFREE_NOUNWIND_READONLY:#[0-9]+]]

 ; CHECK-DARWIN: declare void @memset_pattern16(i8* nocapture writeonly, i8* nocapture readonly, i64) [[ARGMEMONLY_NOFREE_NOUNWIND_WILLRETURN]]
 declare void @memset_pattern16(i8*, i8*, i64)
 
 
 ; CHECK-DAG: attributes [[NOFREE_NOUNWIND_WILLRETURN]] = { mustprogress nofree nounwind willreturn }
 ; CHECK-DAG: attributes [[NOFREE_NOUNWIND_WILLRETURN_WRITEONLY]] = { mustprogress nofree nounwind willreturn writeonly }
 ; CHECK-DAG: attributes [[NOFREE_NOUNWIND]] = { nofree nounwind }
 ; CHECK-DAG: attributes [[INACCESSIBLEMEMONLY_NOFREE_NOUNWIND_WILLRETURN]] = { inaccessiblememonly mustprogress nofree nounwind willreturn }
-; CHECK-DAG: attributes [[INACCESSIBLEMEMONLY_NOFREE_NOUNWIND_WILLRETURN_ALIGN]] = { inaccessiblememonly mustprogress nofree nounwind willreturn allocalign(1) }
 ; CHECK-DAG: attributes [[NOFREE_NOUNWIND_READONLY_WILLRETURN]] = { mustprogress nofree nounwind readonly willreturn }
 ; CHECK-DAG: attributes [[ARGMEMONLY_NOFREE_NOUNWIND_WILLRETURN]] = { argmemonly mustprogress nofree nounwind willreturn }
 ; CHECK-DAG: attributes [[NOFREE_NOUNWIND_READONLY]] = { nofree nounwind readonly }
 ; CHECK-DAG: attributes [[INACCESSIBLEMEMORARGMEMONLY_NOUNWIND_WILLRETURN]] = { inaccessiblemem_or_argmemonly mustprogress nounwind willreturn }
 ; CHECK-DAG: attributes [[NOFREE_WILLRETURN]] = { mustprogress nofree willreturn }
 ; CHECK-DAG: attributes [[ARGMEMONLY_NOFREE_NOUNWIND_READONLY_WILLRETURN]] = { argmemonly mustprogress nofree nounwind readonly willreturn }
 ; CHECK-DAG: attributes [[NOFREE]] = { nofree }
 ; CHECK-DAG: attributes [[INACCESSIBLEMEMORARGONLY_NOFREE_NOUNWIND_WILLRETURN]]  = { inaccessiblemem_or_argmemonly mustprogress nofree nounwind willreturn }
 ; CHECK-DAG: attributes [[ARGMEMONLY_NOFREE_NOUNWIND]] = { argmemonly nofree nounwind }
 
 ; CHECK-NVPTX-DAG: attributes [[NOFREE_NOUNWIND_READNONE]] = { nofree nosync nounwind readnone }