[AST] Generalize argument specific aliasing

[reames]

Summary

AliasSetTracker has special case handling for memset, memcpy and memmove which pre-existed argmemonly on functions and readonly and writeonly on arguments. This patch generalizes it using the AA infrastructure to any call correctly annotated.

The motivation here is to cut down on confusion, not performance per se. For most instructions, there is a direct mapping to alias set. However, this is not guaranteed by the interface and was not in fact true for these three intrinsics *and only these three intrinsics*. I kept getting myself confused about this invariant, so I figured it would be good to clearly distinguish between a instructions and alias sets. Calls happened to be an easy target.

The nice side effect is that custom implementations of memset/memcpy/memmove - including wrappers discovered by IPO - can now be optimized the same as builts by LICM.

[mkazantsev]

I am concerned that these changes didn't change anything in behavior of dedicated tests in test/Analysis/AliasSet. I think we should commit some of those as NFC and then show how this patch changes behavior of these tests.

lib/Analysis/AliasSetTracker.cpp

443

Aren't we supposed to return some value here?

[mkazantsev]

lib/Analysis/AliasSetTracker.cpp

436

How about

if (!isRefSet(ArgMask))
  Access = isModSet(ArgMask) ? AliasSet::ModAccess : AliasSet::NoAccess;

438

Looks more logical to set NoAccess as default and set ModRefAccess if both Ref and Mod are set, but it's up to you.

[hiraditya]

Thanks for the patch.

include/llvm/Analysis/MemoryLocation.h

113

Maybe just remove this function?

lib/Analysis/AliasSetTracker.cpp

443

the function returns void...

lib/Analysis/MemoryLocation.cpp

162

TLI && CS.getCalledFunction() && ....

[reames]

include/llvm/Analysis/MemoryLocation.h

113

I would prefer not to. There are a bunch of callers and I'd like to eventually pipe TLI though here as well. I consider this a temporary step.

[reames]

rebase and incorporate suggestions.

There are a couple of key changes here:

1. I'm intentionally leaving in the redundant memset, and memtransfer pieces. I'll remove this in a following NFC, but the code changes were contributing to rebase problems and weren't strictly needed. Same with removing the invariant_start from addUnknown.
2. I removed the AA change in favour of doing this directly in the caller. This turned out to be needed for invariant_start handling (added in the rebase), and the docs for the function I was modifying contradicted my change anyway. This is much better isolated.
3. All of the tests have landed, so only the updates are shown.

In general, I'm really unhappy with how the write-for-control-flow-only handling (guards, assumes, invariant_starts) is leaking out into more places. For the moment, I'd like to land this as is, but I plan to take a closer look at the framing there and see if we can better abstract this.

[reames]

with the right patch this time

For the record, using ">>" instead of ">" when replacing an existing patch diff leads to a (surprisingly) valid patch, just not with any sensible contents.

[reames]

lib/Analysis/AliasSetTracker.cpp

430

I noticed there's a much better way to get a ModRefInfo from a FuncitonModRefBehavior. I'll replace this stretch of code with a call to createModRefInfo(MRB);

[reames]

rebase for minor issue I commented on, also ping?

[anna]

LGTM.

lib/Analysis/AliasSetTracker.cpp

415

which two family of intrinsics? Are these the memset and memtransfer calls? They are handled above. Maybe move the comment to above line 451.

[reames]

lib/Analysis/AliasSetTracker.cpp

415

The volatile comment is stale and will be removed before landing. AST no longer has a notion of volatile at all.

include/llvm/Analysis/AliasSetTracker.h

 #include <iterator>
 #include <vector>
 
 namespace llvm {
 
 class AliasSetTracker;
 class BasicBlock;
 class LoadInst;
-class AnyMemSetInst;
-class AnyMemTransferInst;
 class raw_ostream;
 class StoreInst;
 class VAArgInst;
 class Value;
 
 class AliasSet : public ilist_node<AliasSet> {
   friend class AliasSetTracker;
 
public:
   ///
   /// These methods return true if inserting the instruction resulted in the
   /// addition of a new alias set (i.e., the pointer did not alias anything).
   ///
   void add(Value *Ptr, LocationSize Size, const AAMDNodes &AAInfo); // Add a loc
   void add(LoadInst *LI);
   void add(StoreInst *SI);
   void add(VAArgInst *VAAI);
-  void add(AnyMemSetInst *MSI);
-  void add(AnyMemTransferInst *MTI);
   void add(Instruction *I);       // Dispatch to one of the other add methods...
   void add(BasicBlock &BB);       // Add all instructions in basic block
   void add(const AliasSetTracker &AST); // Add alias relations from another AST
   void addUnknown(Instruction *I);
 
   void clear();
 
   /// Return the alias sets that are active.

include/llvm/Analysis/MemoryLocation.h

public:
   /// Return a location representing the destination of a memory set or
   /// transfer.
   static MemoryLocation getForDest(const MemIntrinsic *MI);
   static MemoryLocation getForDest(const AtomicMemIntrinsic *MI);
   static MemoryLocation getForDest(const AnyMemIntrinsic *MI);
 
   /// Return a location representing a particular argument of a call.
   static MemoryLocation getForArgument(ImmutableCallSite CS, unsigned ArgIdx,
-                                       const TargetLibraryInfo &TLI);
+                                       const TargetLibraryInfo *TLI);
+  static MemoryLocation getForArgument(ImmutableCallSite CS, unsigned ArgIdx,
+                                       const TargetLibraryInfo &TLI) {
+    return getForArgument(CS, ArgIdx, &TLI);
+  }
 
   explicit MemoryLocation(const Value *Ptr = nullptr,
                           LocationSize Size = UnknownSize,
                           const AAMDNodes &AATags = AAMDNodes())
       : Ptr(Ptr), Size(Size), AATags(AATags) {}
 
   MemoryLocation getWithNewPtr(const Value *NewPtr) const {
     MemoryLocation Copy(*this);

lib/Analysis/AliasAnalysis.cpp

if (AA->pointsToConstantMemory(Loc, OrLocal))
       return true;
 
   return false;
 }
 
 ModRefInfo AAResults::getArgModRefInfo(ImmutableCallSite CS, unsigned ArgIdx) {
   ModRefInfo Result = ModRefInfo::ModRef;
 
+  // Use function information first, then fall back to analyzing each argument
+  // individually 
+  auto MRB = getModRefBehavior(CS);
+  if (MRB == FMRB_DoesNotAccessMemory ||
+      MRB == FMRB_OnlyAccessesInaccessibleMem)
+    return ModRefInfo::NoModRef;
+
+  if (onlyReadsMemory(MRB))
+    Result = clearMod(Result);
+  else if (doesNotReadMemory(MRB))
+    Result = clearRef(Result);
+
   for (const auto &AA : AAs) {
     Result = intersectModRef(Result, AA->getArgModRefInfo(CS, ArgIdx));
 
     // Early-exit the moment we reach the bottom of the lattice.
     if (isNoModRef(Result))
       return ModRefInfo::NoModRef;
   }
 

lib/Analysis/AliasSetTracker.cpp

void AliasSetTracker::add(StoreInst *SI) {
   AliasSet &AS = addPointer(MemoryLocation::get(SI), AliasSet::ModAccess);
   if (SI->isVolatile()) AS.setVolatile();
 }
 
 void AliasSetTracker::add(VAArgInst *VAAI) {
   addPointer(MemoryLocation::get(VAAI), AliasSet::ModRefAccess);
 }
 
-void AliasSetTracker::add(AnyMemSetInst *MSI) {
-  auto MemLoc = MemoryLocation::getForDest(MSI);
-  AliasSet &AS = addPointer(MemLoc, AliasSet::ModAccess);
-  auto *MS = dyn_cast<MemSetInst>(MSI);
-  if (MS && MS->isVolatile())
-    AS.setVolatile();
-}
-
-void AliasSetTracker::add(AnyMemTransferInst *MTI) {
-  auto SrcLoc = MemoryLocation::getForSource(MTI);
-  AliasSet &ASSrc = addPointer(SrcLoc, AliasSet::RefAccess);
-
-  auto DestLoc = MemoryLocation::getForDest(MTI);
-  AliasSet &ASDst = addPointer(DestLoc, AliasSet::ModAccess);
-
-  auto* MT = dyn_cast<MemTransferInst>(MTI);
-  if (MT && MT->isVolatile()) {
-    ASSrc.setVolatile();
-    ASDst.setVolatile();
-  }
-}
-
 void AliasSetTracker::addUnknown(Instruction *Inst) {
   if (isa<DbgInfoIntrinsic>(Inst))
     return; // Ignore DbgInfo Intrinsics.
 
   if (auto *II = dyn_cast<IntrinsicInst>(Inst)) {
     // These intrinsics will show up as affecting memory, but they are just
     // markers.
     switch (II->getIntrinsicID()) {

 void AliasSetTracker::add(Instruction *I) {
   // Dispatch to one of the other add methods.
   if (LoadInst *LI = dyn_cast<LoadInst>(I))
     return add(LI);
   if (StoreInst *SI = dyn_cast<StoreInst>(I))
     return add(SI);
   if (VAArgInst *VAAI = dyn_cast<VAArgInst>(I))
     return add(VAAI);
-  if (AnyMemSetInst *MSI = dyn_cast<AnyMemSetInst>(I))
-    return add(MSI);
-  if (AnyMemTransferInst *MTI = dyn_cast<AnyMemTransferInst>(I))
-    return add(MTI);
+
+  // Handle all calls with known mod/ref sets generically, but special case the
+  // two family of intrinsics which can involve volatile access.
+  bool VolatileAccess = false;
+  if (auto *MS = dyn_cast<MemSetInst>(I))
+    if (MS->isVolatile())
+      VolatileAccess = true;
+  if (auto* MT = dyn_cast<MemTransferInst>(I))
+    if (MT->isVolatile())
+      VolatileAccess = true;
+
+  CallSite CS(I);
+  if (CS && CS.onlyAccessesArgMemory()) {
+    for (auto AI = CS.arg_begin(), AE = CS.arg_end(); AI != AE; ++AI) {
+      const Value *Arg = *AI;
+      if (!Arg->getType()->isPointerTy())
+        continue;
+      unsigned ArgIdx = std::distance(CS.arg_begin(), AI);
+      MemoryLocation ArgLoc = MemoryLocation::getForArgument(CS, ArgIdx,
+                                                             nullptr);
+      ModRefInfo ArgMask = AA.getArgModRefInfo(CS, ArgIdx);
+      auto Access = AliasSet::ModRefAccess;
+      if (isRefSet(ArgMask) && !isModSet(ArgMask))
+        Access = AliasSet::RefAccess;
+      else if (!isRefSet(ArgMask) && isModSet(ArgMask))
+        Access = AliasSet::ModAccess;
+      else if (!isRefSet(ArgMask) && !isModSet(ArgMask))
+        Access = AliasSet::NoAccess;
+      auto &AS = addPointer(ArgLoc, Access);
+      if (VolatileAccess) AS.setVolatile();
+    }
+    return;
+  }  
   return addUnknown(I);
 }
 
 void AliasSetTracker::add(BasicBlock &BB) {
   for (auto &I : BB)
     add(&I);
 }
 

lib/Analysis/MemoryLocation.cpp

MemoryLocation MemoryLocation::getForDest(const AnyMemIntrinsic *MI) {
   AAMDNodes AATags;
   MI->getAAMetadata(AATags);
 
   return MemoryLocation(MI->getRawDest(), Size, AATags);
 }
 
 MemoryLocation MemoryLocation::getForArgument(ImmutableCallSite CS,
                                               unsigned ArgIdx,
-                                              const TargetLibraryInfo &TLI) {
+                                              const TargetLibraryInfo *TLI) {
   AAMDNodes AATags;
   CS->getAAMetadata(AATags);
   const Value *Arg = CS.getArgument(ArgIdx);
 
   // We may be able to produce an exact size for known intrinsics.
   if (const IntrinsicInst *II = dyn_cast<IntrinsicInst>(CS.getInstruction())) {
     const DataLayout &DL = II->getModule()->getDataLayout();
 
if (const IntrinsicInst *II = dyn_cast<IntrinsicInst>(CS.getInstruction())) {
     }
   }
 
   // We can bound the aliasing properties of memset_pattern16 just as we can
   // for memcpy/memset.  This is particularly important because the
   // LoopIdiomRecognizer likes to turn loops into calls to memset_pattern16
   // whenever possible.
   LibFunc F;
-  if (CS.getCalledFunction() && TLI.getLibFunc(*CS.getCalledFunction(), F) &&
-      F == LibFunc_memset_pattern16 && TLI.has(F)) {
+  if (CS.getCalledFunction() && TLI &&
+      TLI->getLibFunc(*CS.getCalledFunction(), F) &&
+      F == LibFunc_memset_pattern16 && TLI->has(F)) {
     assert((ArgIdx == 0 || ArgIdx == 1) &&
            "Invalid argument index for memset_pattern16");
     if (ArgIdx == 1)
       return MemoryLocation(Arg, 16, AATags);
     if (const ConstantInt *LenCI = dyn_cast<ConstantInt>(CS.getArgument(2)))
       return MemoryLocation(Arg, LenCI->getZExtValue(), AATags);
   }
   // FIXME: Handle memset_pattern4 and memset_pattern8 also.
 
   return MemoryLocation(CS.getArgument(ArgIdx), UnknownSize, AATags);
 }

test/Transforms/LICM/argmemonly-call.ll

 ; CHECK: @bar
   br label %loop
 
 loop:
   %res = call i32 @bar(i32* %loc2)
   store i32 %res, i32* %loc
   br label %loop
 }
+
+; memcpy doesn't write to it's source argument, so loads to that location
+; can still be hoisted
+define void @test5(i32* noalias %loc, i32* noalias %loc2) {
+; CHECK-LABEL: @test5
+; CHECK: %val = load i32, i32* %loc2
+; CHECK-LABEL: loop:
+; CHECK: @llvm.memcpy
+  br label %loop
+
+loop:
+  %val = load i32, i32* %loc2
+  store i32 %val, i32* %loc
+  %dest = bitcast i32* %loc to i8*
+  %src = bitcast i32* %loc2 to i8*
+  call void @llvm.memcpy.p0i8.p0i8.i64(i8* %dest, i8* %src, i64 8, i1 false)
+  br label %loop
+}
+
+define void @test6(i32* noalias %loc, i32* noalias %loc2) {
+; CHECK-LABEL: @test6
+; CHECK: %val = load i32, i32* %loc2
+; CHECK-LABEL: loop:
+; CHECK: @custom_memcpy
+  br label %loop
+
+loop:
+  %val = load i32, i32* %loc2
+  store i32 %val, i32* %loc
+  %dest = bitcast i32* %loc to i8*
+  %src = bitcast i32* %loc2 to i8*
+  call void @custom_memcpy(i8* %dest, i8* %src)
+  br label %loop
+}
+
+declare void @llvm.memcpy.p0i8.p0i8.i64(i8* nocapture writeonly, i8* nocapture readonly, i64, i1)
+declare void @custom_memcpy(i8* nocapture writeonly, i8* nocapture readonly) argmemonly nounwind