[MemorySSA] Don't use template when the clone is a simplified instruction.

[asbirlea]

Summary

LoopRotate doesn't create a faithful clone of an instruction, it may
simplify it beforehand. Hence the clone of an instruction that has a
MemoryDef associated may not be a definition, but a use or not a memory
alternig instruction.
Don't rely on the template when the clone may be simplified.

[george.burgess.iv]

just two stylistic nits; lgtm afterward. thanks for this!

include/llvm/Analysis/MemorySSAUpdater.h

292 ↗

(On Diff #204828)

I think that we need to spell out very clearly in the comment what this new parameter allows. I assume it's something along the lines of "If true, the clone was exact. Otherwise, assume that the clone involved simplifications that may have:

• turned a MemoryUse into an instruction that MemorySSA has no representation for, or
• turned a MemoryDef into a MemoryUse or an instruction that MemorySSA has no representation for

No other cases are supported."

In which case, I'd probably spell this bool something like CloneWasSimplified or simialar; useTemplateMUD is inscrutable for those who don't know how this is implemented IMO

lib/Analysis/MemorySSAUpdater.cpp

659 ↗

(On Diff #204828)

please use /*paramName=*/false

[asbirlea]

Address comments.

llvm/trunk/include/llvm/Analysis/MemorySSAUpdater.h

private:
   // instructions and blocks cloned, and a map of MemoryPhi : Definition
   // (MemoryAccess Phi or Def). VMap maps old instructions to cloned
   // instructions and old blocks to cloned blocks. MPhiMap, is created in the
   // caller of this private method, and maps existing MemoryPhis to new
   // definitions that new MemoryAccesses must point to. These definitions may
   // not necessarily be MemoryPhis themselves, they may be MemoryDefs. As such,
   // the map is between MemoryPhis and MemoryAccesses, where the MemoryAccesses
   // may be MemoryPhis or MemoryDefs and not MemoryUses.
+  // If CloneWasSimplified = true, the clone was exact. Otherwise, assume that
+  // the clone involved simplifications that may have: (1) turned a MemoryUse
+  // into an instruction that MemorySSA has no representation for, or (2) turned
+  // a MemoryDef into a MemoryUse or an instruction that MemorySSA has no
+  // representation for. No other cases are supported.
   void cloneUsesAndDefs(BasicBlock *BB, BasicBlock *NewBB,
-                        const ValueToValueMapTy &VMap, PhiToDefMap &MPhiMap);
+                        const ValueToValueMapTy &VMap, PhiToDefMap &MPhiMap,
+                        bool CloneWasSimplified = false);
   template <typename Iter>
   void privateUpdateExitBlocksForClonedLoop(ArrayRef<BasicBlock *> ExitBlocks,
                                             Iter ValuesBegin, Iter ValuesEnd,
                                             DominatorTree &DT);
   void applyInsertUpdates(ArrayRef<CFGUpdate>, DominatorTree &DT,
                           const GraphDiff<BasicBlock *> *GD);
 };
 } // end namespace llvm
 
 #endif // LLVM_ANALYSIS_MEMORYSSAUPDATER_H

llvm/trunk/lib/Analysis/MemorySSAUpdater.cpp

if (MemoryPhi *MPhi = MSSA->getMemoryAccess(To)) {
     });
     if (MPhi->getNumIncomingValues() == 1)
       removeMemoryAccess(MPhi);
   }
 }
 
 void MemorySSAUpdater::cloneUsesAndDefs(BasicBlock *BB, BasicBlock *NewBB,
                                         const ValueToValueMapTy &VMap,
-                                        PhiToDefMap &MPhiMap) {
+                                        PhiToDefMap &MPhiMap,
+                                        bool CloneWasSimplified) {
   auto GetNewDefiningAccess = [&](MemoryAccess *MA) -> MemoryAccess * {
     MemoryAccess *InsnDefining = MA;
     if (MemoryUseOrDef *DefMUD = dyn_cast<MemoryUseOrDef>(InsnDefining)) {
       if (!MSSA->isLiveOnEntryDef(DefMUD)) {
         Instruction *DefMUDI = DefMUD->getMemoryInst();
         assert(DefMUDI && "Found MemoryUseOrDef with no Instruction.");
         if (Instruction *NewDefMUDI =
                 cast_or_null<Instruction>(VMap.lookup(DefMUDI)))
if (!Acc)
     return;
   for (const MemoryAccess &MA : *Acc) {
     if (const MemoryUseOrDef *MUD = dyn_cast<MemoryUseOrDef>(&MA)) {
       Instruction *Insn = MUD->getMemoryInst();
       // Entry does not exist if the clone of the block did not clone all
       // instructions. This occurs in LoopRotate when cloning instructions
       // from the old header to the old preheader. The cloned instruction may
       // also be a simplified Value, not an Instruction (see LoopRotate).
+      // Also in LoopRotate, even when it's an instruction, due to it being
+      // simplified, it may be a Use rather than a Def, so we cannot use MUD as
+      // template. Calls coming from updateForClonedBlockIntoPred, ensure this.
       if (Instruction *NewInsn =
               dyn_cast_or_null<Instruction>(VMap.lookup(Insn))) {
         MemoryAccess *NewUseOrDef = MSSA->createDefinedAccess(
-            NewInsn, GetNewDefiningAccess(MUD->getDefiningAccess()), MUD);
+            NewInsn, GetNewDefiningAccess(MUD->getDefiningAccess()),
+            CloneWasSimplified ? nullptr : MUD);
         MSSA->insertIntoListsForBlock(NewUseOrDef, NewBB, MemorySSA::End);
       }
     }
   }
 }
 
 void MemorySSAUpdater::updatePhisWhenInsertingUniqueBackedgeBlock(
     BasicBlock *Header, BasicBlock *Preheader, BasicBlock *BEBlock) {

 
 void MemorySSAUpdater::updateForClonedBlockIntoPred(
     BasicBlock *BB, BasicBlock *P1, const ValueToValueMapTy &VM) {
   // All defs/phis from outside BB that are used in BB, are valid uses in P1.
   // Since those defs/phis must have dominated BB, and also dominate P1.
   // Defs from BB being used in BB will be replaced with the cloned defs from
   // VM. The uses of BB's Phi (if it exists) in BB will be replaced by the
   // incoming def into the Phi from P1.
+  // Instructions cloned into the predecessor are in practice sometimes
+  // simplified, so disable the use of the template, and create an access from
+  // scratch.
   PhiToDefMap MPhiMap;
   if (MemoryPhi *MPhi = MSSA->getMemoryAccess(BB))
     MPhiMap[MPhi] = MPhi->getIncomingValueForBlock(P1);
-  cloneUsesAndDefs(BB, P1, VM, MPhiMap);
+  cloneUsesAndDefs(BB, P1, VM, MPhiMap, /*CloneWasSimplified=*/true);
 }
 
 template <typename Iter>
 void MemorySSAUpdater::privateUpdateExitBlocksForClonedLoop(
     ArrayRef<BasicBlock *> ExitBlocks, Iter ValuesBegin, Iter ValuesEnd,
     DominatorTree &DT) {
   SmallVector<CFGUpdate, 4> Updates;
   // Update/insert phis in all successors of exit blocks.

llvm/trunk/test/Analysis/MemorySSA/loop-rotate-inv-template.ll

+; RUN: opt -disable-output -loop-rotate -enable-mssa-loop-dependency -verify-memoryssa %s
+; REQUIRES: asserts
+
+; Function Attrs: nounwind
+define dso_local void @bar() local_unnamed_addr #0 align 32 {
+entry:
+  br label %looplabel.exit.i
+
+looplabel.exit.i: ; preds = %if.end.i, %entry
+  %0 = phi i1 (i32*, i32*)* [ @foo, %entry ], [ undef, %if.end.i ]
+  %call3.i.i = call zeroext i1 %0(i32* nonnull dereferenceable(16) undef, i32* nonnull undef)
+  br i1 %call3.i.i, label %if.end.i, label %label.exit
+
+if.end.i:                                         ; preds = %looplabel.exit.i
+  %tobool.i = icmp eq i32* undef, null
+  br label %looplabel.exit.i
+
+label.exit: ; preds = %looplabel.exit.i
+  ret void
+}
+
+; Function Attrs: readonly
+declare dso_local i1 @foo(i32*, i32*) #1 align 32
+
+attributes #0 = { nounwind }
+attributes #1 = { readonly }
+