[PATCH] D22966: GVN-hoist: compute MSSA once per function (PR28670)

[Sebastian Pop via llvm-commits]

sebpop updated this revision to Diff 66212.
sebpop added a comment.

Updated the patch as I first wrote it: I think this is the natural way to update an SSA variable.

This fails because when invoking:
+          MSSA->removeMemoryAccess(OldMemAcc);

we go from:
; MemoryUse(2)

  %2 = load i32, i32* %v.addr, align 4

to no annotation at all:

  %2 = load i32, i32* %v.addr, align 4

This is because removeMemoryAccess calls removeFromLookups which does:

  Value *MemoryInst;
  if (MemoryUseOrDef *MUD = dyn_cast<MemoryUseOrDef>(MA)) {
    MemoryInst = MUD->getMemoryInst();
  } else {
    MemoryInst = MA->getBlock();
  }
  ValueToMemoryAccess.erase(MemoryInst);

and erase() removes both the new and old MemoryAccess'es leaving the instruction without MSSA annotation.

For loads this can be fixed by first removing the OldMemoryAccess and then creating the NewMemoryAccess.
This works for loads because there are no users of a MemoryUse.
However for a store, we need to keep both the old and new MemoryAccess'es at the same time, to be able to rename the uses of old with new.

I think the solution would be for "MSSA->removeMemoryAccess(OldMemAcc);" to only invalidate and remove "OldMemAcc" without touching "NewMemAcc" that should remain attached to the instruction.


https://reviews.llvm.org/D22966

Files:
  lib/Transforms/Scalar/GVNHoist.cpp

Index: lib/Transforms/Scalar/GVNHoist.cpp
===================================================================
--- lib/Transforms/Scalar/GVNHoist.cpp
+++ lib/Transforms/Scalar/GVNHoist.cpp
@@ -193,30 +193,27 @@
     VN.setAliasAnalysis(AA);
     VN.setMemDep(MD);
     bool Res = false;
+    MemorySSA M(F, AA, DT);
+    MSSA = &M;
 
     // FIXME: use lazy evaluation of VN to avoid the fix-point computation.
     while (1) {
-      // FIXME: only compute MemorySSA once. We need to update the analysis in
-      // the same time as transforming the code.
-      MemorySSA M(F, AA, DT);
-      MSSA = &M;
-
       // Perform DFS Numbering of instructions.
       unsigned I = 0;
       for (const BasicBlock *BB : depth_first(&F.getEntryBlock()))
         for (auto &Inst: *BB)
           DFSNumber.insert({&Inst, ++I});
 
       auto HoistStat = hoistExpressions(F);
-      if (HoistStat.first + HoistStat.second == 0) {
+      if (HoistStat.first + HoistStat.second == 0)
         return Res;
-      }
-      if (HoistStat.second > 0) {
+
+      if (HoistStat.second > 0)
         // To address a limitation of the current GVN, we need to rerun the
-        // hoisting after we hoisted loads in order to be able to hoist all
-        // scalars dependent on the hoisted loads. Same for stores.
+        // hoisting after we hoisted loads or stores in order to be able to
+        // hoist all scalars dependent on the hoisted ld/st.
         VN.clear();
-      }
+
       Res = true;
 
       // DFS numbers change when instructions are hoisted: clear and recompute.
@@ -725,6 +722,8 @@
           if (!Repl || firstInBB(I, Repl))
             Repl = I;
 
+      MemoryAccess *NewMemAcc = nullptr;
+
       if (Repl) {
         // Repl is already in HoistPt: it remains in place.
         assert(allOperandsAvailable(Repl, HoistPt) &&
@@ -742,7 +741,23 @@
             !makeGepOperandsAvailable(Repl, HoistPt, InstructionsToHoist))
           continue;
 
+        // Before moving the instruction, get its MSSA access.
+        MemoryUseOrDef *OldMemAcc = nullptr;
+        if (MemoryAccess *MA = MSSA->getMemoryAccess(Repl))
+          OldMemAcc = dyn_cast<MemoryUseOrDef>(MA);
+
+        // Move the instruction at the end of HoistPt.
         Repl->moveBefore(HoistPt->getTerminator());
+
+        if (OldMemAcc) {
+          // The definition of this ld/st will not change: ld/st hoisting is
+          // legal when the ld/st is not moved past its current definition.
+          MemoryAccess *Def = OldMemAcc->getDefiningAccess();
+          NewMemAcc = MSSA->createMemoryAccessInBB(Repl, Def, HoistPt,
+                                                   MemorySSA::End);
+          OldMemAcc->replaceAllUsesWith(NewMemAcc);
+          MSSA->removeMemoryAccess(OldMemAcc);
+        }
       }
 
       if (isa<LoadInst>(Repl))
@@ -775,6 +790,14 @@
           } else if (isa<CallInst>(Repl)) {
             ++NumCallsRemoved;
           }
+
+          if (NewMemAcc) {
+            // Update the uses of the old MSSA access with NewMemAcc.
+            MemoryAccess *OldMA = MSSA->getMemoryAccess(I);
+            OldMA->replaceAllUsesWith(NewMemAcc);
+            MSSA->removeMemoryAccess(OldMA);
+          }
+
           Repl->intersectOptionalDataWith(I);
           combineKnownMetadata(Repl, I);
           I->replaceAllUsesWith(Repl);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D22966.66212.patch
Type: text/x-patch
Size: 3350 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160730/93bf5025/attachment.bin>