BuildLibCalls: add allocalign attributes for memalign and aligned_alloc

[durin42]

Summary

This gets us close to being able to remove a column from the table in
MemoryBuiltins.cpp.

[durin42]

Question for reviewers: if I can do something similar for the aligned versions of ::operator::new in C++, we can remove a column from the table in MemoryBuiltins.cpp. Should that happen in clang, or someplace else? What am I looking for in clang to make that happen?

[reames]

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

[jyknight]

In D117923#3262389, @durin42 wrote:

Question for reviewers: if I can do something similar for the aligned versions of ::operator::new in C++, we can remove a column from the table in MemoryBuiltins.cpp. Should that happen in clang, or someplace else? What am I looking for in clang to make that happen?

I suggest adding the C++ calls here too, especially if that enables removing the column from MemoryBuiltins, as you suggest.

I think only once we've added all of the attributes required to enable us to eliminate the C++ operators from MemoryBuiltins, then we can delete the special handling of C++ operator new in LLVM. and move all the attribute additions into Clang, all at once. (It just seems simpler to deal with by batching it up that way, instead of moving things across the llvm/clang layer piecemeal).

[nikic]

Unlike C allocator functions, we do not annotate operator new in SLC. Operator new annotations are left to clang. I believe you should already find handling for aligned new there and just need to switch it to use the new attribute rather than emitting an alignment assumption.

[jyknight]

In D117923#3301220, @nikic wrote:

Unlike C allocator functions, we do not annotate operator new in SLC. Operator new annotations are left to clang. I believe you should already find handling for aligned new there and just need to switch it to use the new attribute rather than emitting an alignment assumption.

Except that they _are_ currently special-cased in LLVM, just hardcoded in MemoryBuiltins.cpp vs having attributes defined for the behaviors. It seemed simpler to keep the division of labor between LLVM/Clang, until everything can be moved over at once. But, I don't feel strongly about this -- if you feel the opposite is better, OK.

[durin42]

In D117923#3301283, @jyknight wrote:

In D117923#3301220, @nikic wrote:

Unlike C allocator functions, we do not annotate operator new in SLC. Operator new annotations are left to clang. I believe you should already find handling for aligned new there and just need to switch it to use the new attribute rather than emitting an alignment assumption.

Except that they _are_ currently special-cased in LLVM, just hardcoded in MemoryBuiltins.cpp vs having attributes defined for the behaviors. It seemed simpler to keep the division of labor between LLVM/Clang, until everything can be moved over at once. But, I don't feel strongly about this -- if you feel the opposite is better, OK.

There's new patches in the stack now (annotated correctly in phabricator) that add the correct attribute-adding stuff to clang, and then removes the allocalign column from the table in MemoryBuiltins.cpp. PTAL?

[xbolva00]

https://reviews.llvm.org/rG74b1c4c36740fe94953ef648e40df7dd9e9a9c7d

Clang will do the right thing.

[durin42]

In D117923#3305691, @xbolva00 wrote:

https://reviews.llvm.org/rG74b1c4c36740fe94953ef648e40df7dd9e9a9c7d

Clang will do the right thing.

In other words, I should discard this change? Is that right?

[nikic]

I think you'd want to keep this change. It is still relevant for non-clang frontends with an FFI interface. It would seem silly to not infer one particular attribute here while we infer everything else.

[jyknight]

Agree, we do want this. The general strategy (which I don't think should be changed) is for LLVM to be aware of standard libc functions, and infer attributes for them. (We do want to get it out of the business of being aware of the C++ stdlib functions, however.)

[jyknight]

llvm/lib/Transforms/Utils/BuildLibCalls.cpp

226

Just noticed: this was supposed to be hasParamAttribute, not hasFnAttribute.

[nikic]

LGTM

llvm/lib/Transforms/Utils/BuildLibCalls.cpp

226

nit: Drop braces for single-line if.

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.hasParamAttribute(ArgNo, Attribute::AllocAlign))
+    return false;
+  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;
   return inferLibFuncAttributes(*F, TLI);
 }
 
case LibFunc_setitimer:
     return Changed;
   case LibFunc_system:
     // May throw; "system" is a valid pthread cancellation point.
     Changed |= setRetAndArgsNoUndef(F);
     Changed |= setDoesNotCapture(F, 0);
     Changed |= setOnlyReadsMemory(F, 0);
     return Changed;
   case LibFunc_aligned_alloc:
+    Changed |= setAlignedAllocParam(F, 0);
+    LLVM_FALLTHROUGH;
   case LibFunc_valloc:
   case LibFunc_malloc:
   case LibFunc_vec_malloc:
     Changed |= setOnlyAccessesInaccessibleMemory(F);
     Changed |= setRetAndArgsNoUndef(F);
     Changed |= setDoesNotThrow(F);
     Changed |= setRetDoesNotAlias(F);
     Changed |= setWillReturn(F);
case LibFunc_memcpy_chk:
     Changed |= setOnlyAccessesArgMemory(F);
     Changed |= setDoesNotAlias(F, 0);
     Changed |= setOnlyWritesMemory(F, 0);
     Changed |= setDoesNotAlias(F, 1);
     Changed |= setDoesNotCapture(F, 1);
     Changed |= setOnlyReadsMemory(F, 1);
     return Changed;
   case LibFunc_memalign:
+    Changed |= setAlignedAllocParam(F, 0);
     Changed |= setOnlyAccessesInaccessibleMemory(F);
     Changed |= setRetNoUndef(F);
     Changed |= setDoesNotThrow(F);
     Changed |= setRetDoesNotAlias(F);
     Changed |= setWillReturn(F);
     return Changed;
   case LibFunc_mkdir:
     Changed |= setRetAndArgsNoUndef(F);

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:#[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]]
+; 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]+]]