[LoopRotate + MemorySSA] Keep an <instruction-cloned instruction> map.

[asbirlea]

Summary

The map kept in loop rotate is used for instruction remapping, in order
to simplify the clones of instructions. Thus, if an instruction can be
simplified, its simplified value is placed in the map, even when the
clone is added to the IR. MemorySSA in contrast needs to know about that
clone, so it can add an access for it.
To resolve this: keep a different map for MemorySSA.

[george.burgess.iv]

thanks for this!

just a single optional style nit.

lib/Transforms/Utils/LoopRotationUtils.cpp

379 ↗

(On Diff #206097)

are these all intended to turn into inserts, or is the intent to overwrite old values, too?

in the former case, please do the (admittedly obnoxious to type)

bool Inserted = ValueMapMSSA.insert({Inst, C}).second;
(void)Inserted;
assert(Inserted);

unless Foo[X] = Bar; is by far the dominant style in this file

[asbirlea]

lib/Transforms/Utils/LoopRotationUtils.cpp

379 ↗

(On Diff #206097)

I believe all are inserts, but I'm inclined to keep as is, because this holds true for the other ValueMap as well, and the assignment style is used there.

I'd do a refactor NFC for both in a separate patch instead.

llvm/trunk/lib/Transforms/Utils/LoopRotationUtils.cpp

bool LoopRotate::rotateLoop(Loop *L, bool SimplifiedLatch) {
   // Remove any single-entry PHI nodes in it.
   assert(NewHeader->getSinglePredecessor() &&
          "New header doesn't have one pred!");
   FoldSingleEntryPHINodes(NewHeader);
 
   // Begin by walking OrigHeader and populating ValueMap with an entry for
   // each Instruction.
   BasicBlock::iterator I = OrigHeader->begin(), E = OrigHeader->end();
-  ValueToValueMapTy ValueMap;
+  ValueToValueMapTy ValueMap, ValueMapMSSA;
 
   // For PHI nodes, the value available in OldPreHeader is just the
   // incoming value from OldPreHeader.
   for (; PHINode *PN = dyn_cast<PHINode>(I); ++I)
     ValueMap[PN] = PN->getIncomingValueForBlock(OrigPreheader);
 
   // For the rest of the instructions, either hoist to the OrigPreheader if
   // possible or create a clone in the OldPreHeader if not.
while (I != E) {
     if (C) {
       // Otherwise, stick the new instruction into the new block!
       C->setName(Inst->getName());
       C->insertBefore(LoopEntryBranch);
 
       if (auto *II = dyn_cast<IntrinsicInst>(C))
         if (II->getIntrinsicID() == Intrinsic::assume)
           AC->registerAssumption(II);
+      // MemorySSA cares whether the cloned instruction was inserted or not, and
+      // not whether it can be remapped to a simplified value.
+      ValueMapMSSA[Inst] = C;
     }
   }
 
   // Along with all the other instructions, we just cloned OrigHeader's
   // terminator into OrigPreHeader. Fix up the PHI nodes in each of OrigHeader's
   // successors by duplicating their incoming values for OrigHeader.
   for (BasicBlock *SuccBB : successors(OrigHeader))
     for (BasicBlock::iterator BI = SuccBB->begin();
          PHINode *PN = dyn_cast<PHINode>(BI); ++BI)
       PN->addIncoming(PN->getIncomingValueForBlock(OrigHeader), OrigPreheader);
 
   // Now that OrigPreHeader has a clone of OrigHeader's terminator, remove
   // OrigPreHeader's old terminator (the original branch into the loop), and
   // remove the corresponding incoming values from the PHI nodes in OrigHeader.
   LoopEntryBranch->eraseFromParent();
 
   // Update MemorySSA before the rewrite call below changes the 1:1
-  // instruction:cloned_instruction_or_value mapping in ValueMap.
+  // instruction:cloned_instruction_or_value mapping.
   if (MSSAU) {
-    ValueMap[OrigHeader] = OrigPreheader;
-    MSSAU->updateForClonedBlockIntoPred(OrigHeader, OrigPreheader, ValueMap);
+    ValueMapMSSA[OrigHeader] = OrigPreheader;
+    MSSAU->updateForClonedBlockIntoPred(OrigHeader, OrigPreheader,
+                                        ValueMapMSSA);
   }
 
   SmallVector<PHINode*, 2> InsertedPHIs;
   // If there were any uses of instructions in the duplicated block outside the
   // loop, update them, inserting PHI nodes as required
   RewriteUsesOfClonedInstructions(OrigHeader, OrigPreheader, ValueMap,
                                   &InsertedPHIs);
 

llvm/trunk/test/Analysis/MemorySSA/loop-rotate-valuemap.ll

+; RUN: opt -loop-rotate -enable-mssa-loop-dependency %s -S | FileCheck %s
+; REQUIRES: asserts
+
+; Check that loop rotate keeps proper mapping between cloned instructions,
+; otherwise, MemorySSA will assert.
+
+; CHECK-LABEL: @f
+define void @f() {
+entry:
+  br label %for.body16
+
+for.cond.cleanup15:                               ; preds = %for.body16
+  ret void
+
+for.body16:                                       ; preds = %for.body16.for.body16_crit_edge, %entry
+  %call.i = tail call float @expf(float 0.000000e+00) #1
+  %0 = load float*, float** undef, align 8
+  br i1 undef, label %for.cond.cleanup15, label %for.body16.for.body16_crit_edge
+
+for.body16.for.body16_crit_edge:                  ; preds = %for.body16
+  %.pre = load float, float* undef, align 8
+  br label %for.body16
+}
+
+declare float @expf(float)
+