[MemoryBuiltins] Remove isNoAliasFn() in favor of isNoAliasCall()

[nikic]

Summary

We currently have two similar implementations of this concept: isNoAliasCall() only checks for the noalias return attribute. isNoAliasFn() also checks for allocation functions.

We should switch to only checking the attribute. SLC is responsible for inferring the noalias return attribute for non-new allocation functions (with a missing case fixed in https://github.com/llvm/llvm-project/commit/348bc76e3548c52dbcd442590ca0a7f5b09b7534). For new, clang is responsible for setting the attribute, if -fno-assume-sane-operator-new is not passed.

[nikic]

llvm/lib/Transforms/IPO/AttributorAttributes.cpp

5272

This check is already performed within getInitialValueForObj(), I figured we may as well not duplicate the logic.

[jdoerfert]

I like this, anyone else?

llvm/lib/Transforms/IPO/AttributorAttributes.cpp

5272

Agreed.

[fhahn]

LGTM, thanks!

[reames]

LGTM as well.

(You'll have to rebase over a couple of my commits from last night, but should be straightforward. Trying to cleanup this interface as much as possible so we can tell what properties of allocator routines we actually care about.)

llvm/include/llvm/Analysis/MemoryBuiltins.h

 
 /// Tests if a value is a call or invoke to a library function that
 /// allocates or reallocates memory (either malloc, calloc, realloc, or strdup
 /// like).
 bool isAllocationFn(const Value *V, const TargetLibraryInfo *TLI);
 bool isAllocationFn(const Value *V,
                     function_ref<const TargetLibraryInfo &(Function &)> GetTLI);
 
-/// Tests if a value is a call or invoke to a function that returns a
-/// NoAlias pointer (including malloc/calloc/realloc/strdup-like functions).
-bool isNoAliasFn(const Value *V, const TargetLibraryInfo *TLI);
-
 /// Tests if a value is a call or invoke to a library function that
 /// allocates uninitialized memory (such as malloc).
 bool isMallocLikeFn(const Value *V, const TargetLibraryInfo *TLI);
 bool isMallocLikeFn(const Value *V,
                     function_ref<const TargetLibraryInfo &(Function &)> GetTLI);
 
 /// Tests if a value is a call or invoke to a library function that
 /// allocates uninitialized memory with alignment (such as aligned_alloc).

llvm/lib/Analysis/MemoryBuiltins.cpp

static Optional<AllocFnsTy> getAllocationSize(const Value *V,
   // really allowed to assume anything, so we use MallocLike.
   Result.AllocTy = MallocLike;
   Result.NumParams = Callee->getNumOperands();
   Result.FstParam = Args.first;
   Result.SndParam = Args.second.getValueOr(-1);
   return Result;
 }
 
-static bool hasNoAliasAttr(const Value *V) {
-  const auto *CB = dyn_cast<CallBase>(V);
-  return CB && CB->hasRetAttr(Attribute::NoAlias);
-}
-
 /// Tests if a value is a call or invoke to a library function that
 /// allocates or reallocates memory (either malloc, calloc, realloc, or strdup
 /// like).
 bool llvm::isAllocationFn(const Value *V, const TargetLibraryInfo *TLI) {
   return getAllocationData(V, AnyAlloc, TLI).hasValue();
 }
 bool llvm::isAllocationFn(
     const Value *V, function_ref<const TargetLibraryInfo &(Function &)> GetTLI) {
   return getAllocationData(V, AnyAlloc, GetTLI).hasValue();
 }
 
-/// Tests if a value is a call or invoke to a function that returns a
-/// NoAlias pointer (including malloc/calloc/realloc/strdup-like functions).
-bool llvm::isNoAliasFn(const Value *V, const TargetLibraryInfo *TLI) {
-  // it's safe to consider realloc as noalias since accessing the original
-  // pointer is undefined behavior
-  return isAllocationFn(V, TLI) ||
-         hasNoAliasAttr(V);
-}
-
 /// Tests if a value is a call or invoke to a library function that
 /// allocates uninitialized memory (such as malloc).
 bool llvm::isMallocLikeFn(const Value *V, const TargetLibraryInfo *TLI) {
   return getAllocationData(V, MallocLike, TLI).hasValue();
 }
 bool llvm::isMallocLikeFn(
     const Value *V, function_ref<const TargetLibraryInfo &(Function &)> GetTLI) {
   return getAllocationData(V, MallocLike, GetTLI)

llvm/lib/Analysis/MemoryDependenceAnalysis.cpp

while (ScanIt != BB->begin()) {
     }
 
     // If this is an allocation, and if we know that the accessed pointer is to
     // the allocation, return Def.  This means that there is no dependence and
     // the access can be optimized based on that.  For example, a load could
     // turn into undef.  Note that we can bypass the allocation itself when
     // looking for a clobber in many cases; that's an alias property and is
     // handled by BasicAA.
-    if (isa<AllocaInst>(Inst) || isNoAliasFn(Inst, &TLI)) {
+    if (isa<AllocaInst>(Inst) || isNoAliasCall(Inst)) {
       const Value *AccessPtr = getUnderlyingObject(MemLoc.Ptr);
       if (AccessPtr == Inst || BatchAA.isMustAlias(Inst, AccessPtr))
         return MemDepResult::getDef(Inst);
     }
 
     if (isInvariantLoad)
       continue;
 

llvm/lib/Transforms/IPO/Attributor.cpp

auto &NoRecurseAA = A.getAAFor<AANoRecurse>(
       QueryingAA, IRPosition::function(*Scope), DepClassTy::OPTIONAL);
   return NoRecurseAA.isAssumedNoRecurse();
 }
 
 Constant *AA::getInitialValueForObj(Value &Obj, Type &Ty,
                                     const TargetLibraryInfo *TLI) {
   if (isa<AllocaInst>(Obj))
     return UndefValue::get(&Ty);
-  if (isNoAliasFn(&Obj, TLI)) {
+  if (isAllocationFn(&Obj, TLI)) {
     if (isMallocLikeFn(&Obj, TLI) || isAlignedAllocLikeFn(&Obj, TLI))
       return UndefValue::get(&Ty);
     if (isCallocLikeFn(&Obj, TLI))
       return Constant::getNullValue(&Ty);
     return nullptr;
   }
   auto *GV = dyn_cast<GlobalVariable>(&Obj);
   if (!GV || !GV->hasLocalLinkage())
if (!AA::getAssumedUnderlyingObjects(A, Ptr, Objects, QueryingAA, &SI)) {
     LLVM_DEBUG(
         dbgs() << "Underlying objects stored into could not be determined\n";);
     return false;
   }
 
   SmallVector<const AAPointerInfo *> PIs;
   SmallVector<Value *> NewCopies;
 
-  const auto *TLI =
-      A.getInfoCache().getTargetLibraryInfoForFunction(*SI.getFunction());
   for (Value *Obj : Objects) {
     LLVM_DEBUG(dbgs() << "Visit underlying object " << *Obj << "\n");
     if (isa<UndefValue>(Obj))
       continue;
     if (isa<ConstantPointerNull>(Obj)) {
       // A null pointer access can be undefined but any offset from null may
       // be OK. We do not try to optimize the latter.
       if (!NullPointerIsDefined(SI.getFunction(),
                                 Ptr.getType()->getPointerAddressSpace()) &&
           A.getAssumedSimplified(Ptr, QueryingAA, UsedAssumedInformation) ==
               Obj)
         continue;
       LLVM_DEBUG(
           dbgs() << "Underlying object is a valid nullptr, giving up.\n";);
       return false;
     }
     if (!isa<AllocaInst>(Obj) && !isa<GlobalVariable>(Obj) &&
-        !isNoAliasFn(Obj, TLI)) {
+        !isNoAliasCall(Obj)) {
       LLVM_DEBUG(dbgs() << "Underlying object is not supported yet: " << *Obj
                         << "\n";);
       return false;
     }
     if (auto *GV = dyn_cast<GlobalVariable>(Obj))
       if (!GV->hasLocalLinkage()) {
         LLVM_DEBUG(dbgs() << "Underlying object is global with external "
                              "linkage, not supported yet: "

llvm/lib/Transforms/IPO/AttributorAttributes.cpp

for (Value *Obj : Objects) {
         // be OK. We do not try to optimize the latter.
         bool UsedAssumedInformation = false;
         if (!NullPointerIsDefined(L.getFunction(),
                                   Ptr.getType()->getPointerAddressSpace()) &&
             A.getAssumedSimplified(Ptr, AA, UsedAssumedInformation) == Obj)
           continue;
         return false;
       }
-      if (!isa<AllocaInst>(Obj) && !isa<GlobalVariable>(Obj) &&
-          !isNoAliasFn(Obj, TLI))
-        return false;
       Constant *InitialVal = AA::getInitialValueForObj(*Obj, *L.getType(), TLI);
       if (!InitialVal || !Union(*InitialVal))
         return false;
 
       LLVM_DEBUG(dbgs() << "Underlying object amenable to load-store "
                            "propagation, checking accesses next.\n");
 
       auto CheckAccess = [&](const AAPointerInfo::Access &Acc, bool IsExact) {

llvm/test/Transforms/GVN/malloc-load-removal.ll

 ; RUN: opt -S -basic-aa -gvn < %s | FileCheck %s
 ; RUN: opt -S -basic-aa -gvn -disable-simplify-libcalls < %s | FileCheck %s -check-prefix=CHECK_NO_LIBCALLS
 ; PR13694
 
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
 target triple = "x86_64-apple-macosx10.8.0"
 
-declare i8* @malloc(i64) nounwind
+declare noalias i8* @malloc(i64) nounwind
 
 define noalias i8* @test1() nounwind uwtable ssp {
 entry:
   %call = tail call i8* @malloc(i64 100) nounwind
   %0 = load i8, i8* %call, align 1
   %tobool = icmp eq i8 %0, 0
   br i1 %tobool, label %if.end, label %if.then
 
 if.then:                                          ; preds = %entry
   store i8 0, i8* %call, align 1
   br label %if.end
 
 if.end:                                           ; preds = %if.then, %entry
   ret i8* %call
 
 ; CHECK-LABEL: @test1(
 ; CHECK-NOT: load
 ; CHECK-NOT: icmp
 
 ; CHECK_NO_LIBCALLS-LABEL: @test1(
 ; CHECK_NO_LIBCALLS: load
 ; CHECK_NO_LIBCALLS: icmp
 }
 
-declare i8* @_Znwm(i64) nounwind
+declare noalias i8* @_Znwm(i64) nounwind
 
 define noalias i8* @test2() nounwind uwtable ssp {
 entry:
   %call = tail call i8* @_Znwm(i64 100) nounwind
   %0 = load i8, i8* %call, align 1
   %tobool = icmp eq i8 %0, 0
   br i1 %tobool, label %if.end, label %if.then
 
 if.then:                                          ; preds = %entry
   store i8 0, i8* %call, align 1
   br label %if.end
 
 if.end:                                           ; preds = %if.then, %entry
   ret i8* %call
 
 ; CHECK-LABEL: @test2(
 ; CHECK-NOT: load
 ; CHECK-NOT: icmp
 
 ; CHECK_NO_LIBCALLS-LABEL: @test2(
 ; CHECK_NO_LIBCALLS: load
 ; CHECK_NO_LIBCALLS: icmp
 }
 
-declare i8* @aligned_alloc(i64, i64) nounwind
+declare noalias i8* @aligned_alloc(i64, i64) nounwind
 
 define noalias i8* @test3() nounwind uwtable ssp {
 entry:
   %call = tail call i8* @aligned_alloc(i64 256, i64 32) nounwind
   %0 = load i8, i8* %call, align 32
   %tobool = icmp eq i8 %0, 0
   br i1 %tobool, label %if.end, label %if.then
 

llvm/unittests/Analysis/MemoryBuiltinsTest.cpp

TEST(AllocSize, AllocationBuiltinsTest) {
 
   AllocSizeFn->addFnAttr(Attribute::getWithAllocSizeArgs(Context, 1, None));
 
   // 100 is arbitrary.
   std::unique_ptr<CallInst> Caller(
       CallInst::Create(AllocSizeFn, {ConstantInt::get(ArgTy, 100)}));
 
   const TargetLibraryInfo *TLI = nullptr;
-  EXPECT_FALSE(isNoAliasFn(Caller.get(), TLI));
   EXPECT_FALSE(isMallocLikeFn(Caller.get(), TLI));
   EXPECT_FALSE(isCallocLikeFn(Caller.get(), TLI));
   EXPECT_FALSE(isAllocLikeFn(Caller.get(), TLI));
 
   // FIXME: We might be able to treat allocsize functions as general allocation
-  // functions. For the moment, being conservative seems better (and we'd have
-  // to plumb stuff around `isNoAliasFn`).
+  // functions.
   EXPECT_FALSE(isAllocationFn(Caller.get(), TLI));
 }
 }