Print OptimizedAccessType in MemorySSA if it is set

[aqjune]

Summary

This patch makes MemoryUse/MemoryDef of MemorySSA print OptimizedAccessType if they're set.

Any comments are welcome.

[george.burgess.iv]

Hi, and thanks for this patch!

I'm unsure if we want to print this out for Defs without adding a bit more information to MemoryDef::print. This is because MemoryUses and MemoryDefs both print out their defining accesses, and getOptimizedAccessType() returns information about the optimized access of a Use or Def.

For Uses, the optimized access *is* the defining access, so I think this is a great addition. For Defs, we don't really print whether or not it's optimized at all right now; it seems a bit awkward to say "the optimized Access type is MayAlias, but we're not going to tell you what the optimized access is".

So, please either back out the change to MemoryDef::print, or have MemoryDef::print print out where the Def is optimized to, if it's been optimized. I'm happy with either of these. :)

lib/Analysis/MemorySSA.cpp

1848 ↗

(On Diff #150235)

Hm. Does it break anything to make this function a raw_ostream &operator<<(raw_ostream &, AliasResult) that we stick into lib/Analysis/AliasAnalysis.cpp (with a declaration in include/llvm/Analysis/AliasAnalysis.cpp)?

I ask because this seems useful to just about anyone who wants to print an AliasResult. :)

1851 ↗

(On Diff #150235)

I suspect this isn't clang-format'ed. If not, please do so.

1858 ↗

(On Diff #150235)

Do we need the default case here? If not, please remove it: I'd rather have compilers warn about missing cases if we ever add something else to AliasResult (unlikely, but...)

1880 ↗

(On Diff #150235)

Style nit: Please fold this into the if, like so:

if (Optional<AliasResult> AR = getOptimizedAccessType()) {
  // ...
}

1921 ↗

(On Diff #150235)

(Same style nit)

[aqjune]

• Define << operator for AliasResult & let AliasAnalysisEvaluator.cpp use it as well
• Update printing format of MemoryDef so it prints optimized def as well

[aqjune]

Comments applied :)

[george.burgess.iv]

This LGTM after one last ultra-small nitpick is addressed. I don't see you in the commit logs, so will you need me to commit this for you?

include/llvm/Analysis/AliasAnalysis.h

95 ↗

(On Diff #150669)

Nit: I think we generally try to define these out-of-line unless their body is literally { Foo.print(OS); return OS; }. Please do so.

lib/Analysis/AliasAnalysisEvaluator.cpp

153 ↗

(On Diff #150669)

Ooh, nice :)

[george.burgess.iv]

(Sorry for the double-post; one more bit that I noticed scrolling up)

lib/Analysis/MemorySSA.cpp

1878 ↗

(On Diff #150669)

(This should probably go before the isOptimized check, so we're consistent with MemoryUses?)

[aqjune]

• Move definition of operator<< to AliasAnalysis.cpp
• Update format of MemoryDef print

[aqjune]

include/llvm/Analysis/AliasAnalysis.h

95 ↗

(On Diff #151094)

I was following the way operator<< is defined in Value.h but I'm not against this suggestion. I updated it

lib/Analysis/MemorySSA.cpp

1879 ↗

(On Diff #151094)

Makes sense, updated it

[aqjune]

Yes, you can commit this patch.

[george.burgess.iv]

This revision was not accepted when it landed; it landed in state Needs Review.

TIL phab will complain about this. To save any code archaeologists some time, this LGTM.

Committed as noted by Phab. Thanks again!

llvm/trunk/include/llvm/Analysis/AliasAnalysis.h

enum AliasResult : uint8_t {
   /// The two locations may or may not alias. This is the least precise result.
   MayAlias,
   /// The two locations alias, but only due to a partial overlap.
   PartialAlias,
   /// The two locations precisely alias each other.
   MustAlias,
 };
 
+/// << operator for AliasResult.
+raw_ostream &operator<<(raw_ostream &OS, AliasResult AR);
+
 /// Flags indicating whether a memory access modifies or references memory.
 ///
 /// This is no access at all, a modification, a reference, or both
 /// a modification and a reference. These are specifically structured such that
 /// they form a three bit matrix and bit-tests for 'mod' or 'ref' or 'must'
 /// work with any of the possible values.
 enum class ModRefInfo : uint8_t {
   /// Must is provided for completeness, but no routines will return only

llvm/trunk/lib/Analysis/AliasAnalysis.cpp

for (const auto &AA : AAs) {
     // Early-exit the moment we reach the bottom of the lattice.
     if (Result == FMRB_DoesNotAccessMemory)
       return Result;
   }
 
   return Result;
 }
 
+raw_ostream &llvm::operator<<(raw_ostream &OS, AliasResult AR) {
+  switch (AR) {
+  case NoAlias:
+    OS << "NoAlias";
+    break;
+  case MustAlias:
+    OS << "MustAlias";
+    break;
+  case MayAlias:
+    OS << "MayAlias";
+    break;
+  case PartialAlias:
+    OS << "PartialAlias";
+    break;
+  }
+  return OS;
+}
+
 //===----------------------------------------------------------------------===//
 // Helper method implementation
 //===----------------------------------------------------------------------===//
 
 ModRefInfo AAResults::getModRefInfo(const LoadInst *L,
                                     const MemoryLocation &Loc) {
   // Be conservative in the face of atomic.
   if (isStrongerThan(L->getOrdering(), AtomicOrdering::Unordered))

llvm/trunk/lib/Analysis/AliasAnalysisEvaluator.cpp

 static cl::opt<bool> PrintModRef("print-modref", cl::ReallyHidden);
 static cl::opt<bool> PrintMust("print-must", cl::ReallyHidden);
 static cl::opt<bool> PrintMustRef("print-mustref", cl::ReallyHidden);
 static cl::opt<bool> PrintMustMod("print-mustmod", cl::ReallyHidden);
 static cl::opt<bool> PrintMustModRef("print-mustmodref", cl::ReallyHidden);
 
 static cl::opt<bool> EvalAAMD("evaluate-aa-metadata", cl::ReallyHidden);
 
-static void PrintResults(const char *Msg, bool P, const Value *V1,
+static void PrintResults(AliasResult AR, bool P, const Value *V1,
                          const Value *V2, const Module *M) {
   if (PrintAll || P) {
     std::string o1, o2;
     {
       raw_string_ostream os1(o1), os2(o2);
       V1->printAsOperand(os1, true, M);
       V2->printAsOperand(os2, true, M);
     }
-    
     if (o2 < o1)
       std::swap(o1, o2);
-    errs() << "  " << Msg << ":\t"
-           << o1 << ", "
-           << o2 << "\n";
+    errs() << "  " << AR << ":\t" << o1 << ", " << o2 << "\n";
   }
 }
 
-static inline void
-PrintModRefResults(const char *Msg, bool P, Instruction *I, Value *Ptr,
-                   Module *M) {
+static inline void PrintModRefResults(const char *Msg, bool P, Instruction *I,
+                                      Value *Ptr, Module *M) {
   if (PrintAll || P) {
     errs() << "  " << Msg << ":  Ptr: ";
     Ptr->printAsOperand(errs(), true, M);
     errs() << "\t<->" << *I << '\n';
   }
 }
 
-static inline void
-PrintModRefResults(const char *Msg, bool P, CallSite CSA, CallSite CSB,
-                   Module *M) {
+static inline void PrintModRefResults(const char *Msg, bool P, CallSite CSA,
+                                      CallSite CSB, Module *M) {
   if (PrintAll || P) {
-    errs() << "  " << Msg << ": " << *CSA.getInstruction()
-           << " <-> " << *CSB.getInstruction() << '\n';
+    errs() << "  " << Msg << ": " << *CSA.getInstruction() << " <-> "
+           << *CSB.getInstruction() << '\n';
   }
 }
 
-static inline void
-PrintLoadStoreResults(const char *Msg, bool P, const Value *V1,
-                      const Value *V2, const Module *M) {
+static inline void PrintLoadStoreResults(AliasResult AR, bool P,
+                                         const Value *V1, const Value *V2,
+                                         const Module *M) {
   if (PrintAll || P) {
-    errs() << "  " << Msg << ": " << *V1
-           << " <-> " << *V2 << '\n';
+    errs() << "  " << AR << ": " << *V1 << " <-> " << *V2 << '\n';
   }
 }
 
 static inline bool isInterestingPointer(Value *V) {
   return V->getType()->isPointerTy()
       && !isa<ConstantPointerNull>(V);
 }
 
for (SetVector<Value *>::iterator I1 = Pointers.begin(), E = Pointers.end();
     Type *I1ElTy = cast<PointerType>((*I1)->getType())->getElementType();
     if (I1ElTy->isSized()) I1Size = DL.getTypeStoreSize(I1ElTy);
 
     for (SetVector<Value *>::iterator I2 = Pointers.begin(); I2 != I1; ++I2) {
       uint64_t I2Size = MemoryLocation::UnknownSize;
       Type *I2ElTy =cast<PointerType>((*I2)->getType())->getElementType();
       if (I2ElTy->isSized()) I2Size = DL.getTypeStoreSize(I2ElTy);
 
-      switch (AA.alias(*I1, I1Size, *I2, I2Size)) {
+      AliasResult AR = AA.alias(*I1, I1Size, *I2, I2Size);
+      switch (AR) {
       case NoAlias:
-        PrintResults("NoAlias", PrintNoAlias, *I1, *I2, F.getParent());
+        PrintResults(AR, PrintNoAlias, *I1, *I2, F.getParent());
         ++NoAliasCount;
         break;
       case MayAlias:
-        PrintResults("MayAlias", PrintMayAlias, *I1, *I2, F.getParent());
+        PrintResults(AR, PrintMayAlias, *I1, *I2, F.getParent());
         ++MayAliasCount;
         break;
       case PartialAlias:
-        PrintResults("PartialAlias", PrintPartialAlias, *I1, *I2,
-                     F.getParent());
+        PrintResults(AR, PrintPartialAlias, *I1, *I2, F.getParent());
         ++PartialAliasCount;
         break;
       case MustAlias:
-        PrintResults("MustAlias", PrintMustAlias, *I1, *I2, F.getParent());
+        PrintResults(AR, PrintMustAlias, *I1, *I2, F.getParent());
         ++MustAliasCount;
         break;
       }
     }
   }
 
   if (EvalAAMD) {
     // iterate over all pairs of load, store
     for (Value *Load : Loads) {
       for (Value *Store : Stores) {
-        switch (AA.alias(MemoryLocation::get(cast<LoadInst>(Load)),
-                         MemoryLocation::get(cast<StoreInst>(Store)))) {
+        AliasResult AR = AA.alias(MemoryLocation::get(cast<LoadInst>(Load)),
+                                  MemoryLocation::get(cast<StoreInst>(Store)));
+        switch (AR) {
         case NoAlias:
-          PrintLoadStoreResults("NoAlias", PrintNoAlias, Load, Store,
-                                F.getParent());
+          PrintLoadStoreResults(AR, PrintNoAlias, Load, Store, F.getParent());
           ++NoAliasCount;
           break;
         case MayAlias:
-          PrintLoadStoreResults("MayAlias", PrintMayAlias, Load, Store,
-                                F.getParent());
+          PrintLoadStoreResults(AR, PrintMayAlias, Load, Store, F.getParent());
           ++MayAliasCount;
           break;
         case PartialAlias:
-          PrintLoadStoreResults("PartialAlias", PrintPartialAlias, Load, Store,
-                                F.getParent());
+          PrintLoadStoreResults(AR, PrintPartialAlias, Load, Store, F.getParent());
           ++PartialAliasCount;
           break;
         case MustAlias:
-          PrintLoadStoreResults("MustAlias", PrintMustAlias, Load, Store,
-                                F.getParent());
+          PrintLoadStoreResults(AR, PrintMustAlias, Load, Store, F.getParent());
           ++MustAliasCount;
           break;
         }
       }
     }
 
     // iterate over all pairs of store, store
     for (SetVector<Value *>::iterator I1 = Stores.begin(), E = Stores.end();
          I1 != E; ++I1) {
       for (SetVector<Value *>::iterator I2 = Stores.begin(); I2 != I1; ++I2) {
-        switch (AA.alias(MemoryLocation::get(cast<StoreInst>(*I1)),
-                         MemoryLocation::get(cast<StoreInst>(*I2)))) {
+        AliasResult AR = AA.alias(MemoryLocation::get(cast<StoreInst>(*I1)),
+                                  MemoryLocation::get(cast<StoreInst>(*I2)));
+        switch (AR) {
         case NoAlias:
-          PrintLoadStoreResults("NoAlias", PrintNoAlias, *I1, *I2,
-                                F.getParent());
+          PrintLoadStoreResults(AR, PrintNoAlias, *I1, *I2, F.getParent());
           ++NoAliasCount;
           break;
         case MayAlias:
-          PrintLoadStoreResults("MayAlias", PrintMayAlias, *I1, *I2,
-                                F.getParent());
+          PrintLoadStoreResults(AR, PrintMayAlias, *I1, *I2, F.getParent());
           ++MayAliasCount;
           break;
         case PartialAlias:
-          PrintLoadStoreResults("PartialAlias", PrintPartialAlias, *I1, *I2,
-                                F.getParent());
+          PrintLoadStoreResults(AR, PrintPartialAlias, *I1, *I2, F.getParent());
           ++PartialAliasCount;
           break;
         case MustAlias:
-          PrintLoadStoreResults("MustAlias", PrintMustAlias, *I1, *I2,
-                                F.getParent());
+          PrintLoadStoreResults(AR, PrintMustAlias, *I1, *I2, F.getParent());
           ++MustAliasCount;
           break;
         }
       }
     }
   }
 
   // Mod/ref alias analysis: compare all pairs of calls and values

llvm/trunk/lib/Analysis/MemorySSA.cpp

void MemoryAccess::print(raw_ostream &OS) const {
   case MemoryUseVal: return static_cast<const MemoryUse *>(this)->print(OS);
   }
   llvm_unreachable("invalid value id");
 }
 
 void MemoryDef::print(raw_ostream &OS) const {
   MemoryAccess *UO = getDefiningAccess();
 
-  OS << getID() << " = MemoryDef(";
-  if (UO && UO->getID())
-    OS << UO->getID();
+  auto printID = [&OS](MemoryAccess *A) {
+    if (A && A->getID())
+      OS << A->getID();
     else
       OS << LiveOnEntryStr;
-  OS << ')';
+  };
+
+  OS << getID() << " = MemoryDef(";
+  printID(UO);
+  OS << ")";
+
+  if (isOptimized()) {
+    OS << "->";
+    printID(getOptimized());
+
+    if (Optional<AliasResult> AR = getOptimizedAccessType())
+      OS << " " << *AR;
+  }
 }
 
 void MemoryPhi::print(raw_ostream &OS) const {
   bool First = true;
   OS << getID() << " = MemoryPhi(";
   for (const auto &Op : operands()) {
     BasicBlock *BB = getIncomingBlock(Op);
     MemoryAccess *MA = cast<MemoryAccess>(Op);

 void MemoryUse::print(raw_ostream &OS) const {
   MemoryAccess *UO = getDefiningAccess();
   OS << "MemoryUse(";
   if (UO && UO->getID())
     OS << UO->getID();
   else
     OS << LiveOnEntryStr;
   OS << ')';
+
+  if (Optional<AliasResult> AR = getOptimizedAccessType())
+    OS << " " << *AR;
 }
 
 void MemoryAccess::dump() const {
 // Cannot completely remove virtual function even in release mode.
 #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
   print(dbgs());
   dbgs() << "\n";
 #endif

llvm/trunk/test/Analysis/MemorySSA/optimize-use.ll

; CHECK-NEXT:   %call1 = call noalias i8* @_Znwm(i64 4)
   %call1 = call noalias i8* @_Znwm(i64 4)
   %1 = bitcast i8* %call1 to i32*
 ; CHECK: 3 = MemoryDef(2)
 ; CHECK-NEXT:   store i32 5, i32* %0, align 4
   store i32 5, i32* %0, align 4
 ; CHECK: 4 = MemoryDef(3)
 ; CHECK-NEXT:   store i32 7, i32* %1, align 4
   store i32 7, i32* %1, align 4
-; CHECK: MemoryUse(3)
+; CHECK: MemoryUse(3) MustAlias
 ; CHECK-NEXT:   %2 = load i32, i32* %0, align 4
   %2 = load i32, i32* %0, align 4
-; CHECK: MemoryUse(4)
+; CHECK: MemoryUse(4) MustAlias
 ; CHECK-NEXT:   %3 = load i32, i32* %1, align 4
   %3 = load i32, i32* %1, align 4
-; CHECK: MemoryUse(3)
+; CHECK: MemoryUse(3) MustAlias
 ; CHECK-NEXT:   %4 = load i32, i32* %0, align 4
   %4 = load i32, i32* %0, align 4
-; CHECK: MemoryUse(4)
+; CHECK: MemoryUse(4) MustAlias
 ; CHECK-NEXT:   %5 = load i32, i32* %1, align 4
   %5 = load i32, i32* %1, align 4
   %add = add nsw i32 %3, %5
   ret i32 %add
 }
 
 declare noalias i8* @_Znwm(i64)