[MemorySSA] Invoke the right getModRefInfo check in `instructionClobbersQuery` when UseLoc is passed.

[bryant]

Summary

Calls to instructionClobbersQuery are meant to answer two kinds of queries:
Given two memory instructions X and Y,

• Does X clobber Y?
• Does X clobber Y with respect to a MemoryLocation UseLoc?

For queries of the first kind, a null MemoryLocation is passed, and it makes
sense for getModRefInfo(Instruction *, ImmutableCallSite) to be called.
For the second kind, however, UseLoc would be a non-null and the above call to
getModRefInfo would be unnecessarily conservative; it would be sufficient to
check getModRefInfo(Instruction *, const MemoryLocation &).

An example:

call void @llvm.memcpy(i8* %dest, i8* %src, i64 128, ...)   ; MemoryDef *M0
call void @llvm.memcpy(i8* %dest, i8* %src, i64 128, ...)   ; MemoryDef *M1

The call

instructionClobbersQuery(M0, MemoryLocation::getForSource(M1->getMemoryInst()),
                         M1->getMemoryInst(), AA)

would return true -- that M0 does clobber M1's source. But this can't possibly
be the case since overlapping memcpy operands are UB.

[• dberlin]

This looks right until we clean up getModRefInfo's interface :P

[bryant]

Actually, can you please add a test for this before you commit?
You can use -print-memoryssa -analyze to get it to dump memoryssa.
If you need specific API calls, please add to the unit tests for MemorySSA.

I'm having trouble getting -analyze to work:

16:58:24 ~/3rd/llvm/test> opt -print-memoryssa -analyze < Transforms/Util/MemorySSA/precision-call-clobbers.ll
... # stuff
Printing analysis 'Memory SSA Printer' for function 'f':
Pass::print not implemented for pass: 'Memory SSA Printer'!

If the goal is to run FileCheck on the MemorySSA debug output, shouldn't -print-memoryssa alone be sufficient?

[bryant]

Added clobber tests.

lib/Transforms/Utils/MemorySSA.cpp

if (const IntrinsicInst *II = dyn_cast<IntrinsicInst>(DefInst)) {
     case Intrinsic::assume:
       return false;
     default:
       break;
     }
   }
 
   ImmutableCallSite UseCS(UseInst);
-  if (UseCS) {
+  if (UseCS && UseLoc == MemoryLocation()) {
     ModRefInfo I = AA.getModRefInfo(DefInst, UseCS);
     return I != MRI_NoModRef;
   }
 
   if (auto *DefLoad = dyn_cast<LoadInst>(DefInst)) {
     if (auto *UseLoad = dyn_cast<LoadInst>(UseInst)) {
       switch (getLoadReorderability(UseLoad, DefLoad)) {
       case Reorderability::Always:

unittests/Transforms/Utils/MemorySSA.cpp

 //===- MemorySSA.cpp - Unit tests for MemorySSA ---------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 #include "llvm/Transforms/Utils/MemorySSA.h"
 #include "llvm/Analysis/AliasAnalysis.h"
 #include "llvm/Analysis/BasicAliasAnalysis.h"
 #include "llvm/IR/BasicBlock.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/Dominators.h"
 #include "llvm/IR/IRBuilder.h"
 #include "llvm/IR/Instructions.h"
+#include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/LLVMContext.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
 
 const static char DLString[] = "e-i64:64-f80:128-n8:16:32:64-S128";
 
 /// There's a lot of common setup between these tests. This fixture helps reduce
TEST_F(MemorySSATest, WalkerReopt) {
 
   // Create the load memory access pointing to an unoptimized place.
   MemoryUse *NewLoadAccess = cast<MemoryUse>(MSSA.createMemoryAccessInBB(
       LIA, MSSA.getMemoryAccess(SIB), LIA->getParent(), MemorySSA::End));
   // This should it cause it to be optimized
   EXPECT_EQ(Walker->getClobberingMemoryAccess(NewLoadAccess), LoadClobber);
   EXPECT_EQ(NewLoadAccess->getDefiningAccess(), LoadClobber);
 }
+
+TEST_F(MemorySSATest, InstClobbersMemLoc) {
+  F = Function::Create(FunctionType::get(B.getVoidTy(),
+                                         {B.getInt8PtrTy(), B.getInt8PtrTy()},
+                                         false),
+                       GlobalValue::ExternalLinkage, "F", &M);
+  B.SetInsertPoint(BasicBlock::Create(C, "", F));
+  Argument *P = &*F->arg_begin();
+  Argument *Q = &*std::next(F->arg_begin());
+
+  Type *Int64 = Type::getInt64Ty(C);
+
+  AllocaInst *A =
+      B.CreateAlloca(Type::getInt128Ty(C), ConstantInt::get(Int64, 1), "A");
+
+  auto *M0 = B.CreateMemCpy(A, P, ConstantInt::get(Int64, 16), 1);
+  auto *M1 =
+      cast<MemCpyInst>(B.CreateMemCpy(Q, A, ConstantInt::get(Int64, 16), 1));
+  auto *M2 =
+      cast<MemCpyInst>(B.CreateMemCpy(Q, A, ConstantInt::get(Int64, 16), 1));
+  auto *M3 =
+      cast<MemCpyInst>(B.CreateMemCpy(Q, P, ConstantInt::get(Int64, 16), 1));
+
+  setupAnalyses();
+  MemorySSA &MSSA = *Analyses->MSSA;
+  MemorySSAWalker &Walker = *Analyses->Walker;
+
+  // M1 obviously clobbers M2 -- on the dest loc.
+  EXPECT_EQ(MSSA.getMemoryAccess(M1), Walker.getClobberingMemoryAccess(M2));
+  // But only M0 clobbers M2 on its source loc. In other words,
+  // instructionClobbersQuery must call getModRefInfo(ImmutableCallSite, const
+  // MemoryLocation).
+  EXPECT_EQ(MSSA.getMemoryAccess(M0),
+            Walker.getClobberingMemoryAccess(MSSA.getMemoryAccess(M2),
+                                             MemoryLocation::getForSource(M2)));
+  // FIXME: AA should know that memcpy operands never overlap.
+  EXPECT_NE(MSSA.getLiveOnEntryDef(),
+            Walker.getClobberingMemoryAccess(MSSA.getMemoryAccess(M3),
+                                             MemoryLocation::getForSource(M3)));
+}