[MemoryBuiltins] Fix an issue with hasNoAliasAttr

[sanjoy]

Summary

hasNoAliasAttr is buggy: it checks to see if the called function has
a noalias attribute, which is incorrect since functions are not even
allowed to have the noalias attribute. The comment on its only
caller, llvm::isNoAliasFn, makes it pretty clear that the intention
to do the noalias check on the return value, and not the callee.

Unfortunately I couldn't find a way to test this upstream -- fixing
this does not change the observable behavior of any of the passes that
use this. This is not very surprising, since noalias does not tell
anything about the contents of the allocated memory (so, e.g., you
still cannot fold loads). I'll be happy to be proven wrong though.

[reames]

LGTM w/optional comments.

As a test case consider:
%p = ...
load from %p
%p2 = malloc()
load from %p

GVN? Not sure this will actually work, but worth trying.

lib/Analysis/MemoryBuiltins.cpp

147 ↗

(On Diff #47349)

Can you add an assert inside the attribute mechanism that would have caught this?

[sanjoy]

lib/Analysis/MemoryBuiltins.cpp

147 ↗

(On Diff #47349)

That's a good idea, I'll do that in a follow-up patch.

llvm/trunk/lib/Analysis/MemoryBuiltins.cpp

if (FTy->getReturnType() == Type::getInt8PtrTy(FTy->getContext()) &&
        FTy->getParamType(SndParam)->isIntegerTy(32) ||
        FTy->getParamType(SndParam)->isIntegerTy(64)))
     return FnData;
   return nullptr;
 }
 
 static bool hasNoAliasAttr(const Value *V, bool LookThroughBitCast) {
   ImmutableCallSite CS(LookThroughBitCast ? V->stripPointerCasts() : V);
-  return CS && CS.hasFnAttr(Attribute::NoAlias);
+  return CS && CS.paramHasAttr(AttributeSet::ReturnIndex, Attribute::NoAlias);
 }
 
 
 /// \brief 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,
                           bool LookThroughBitCast) {