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

[Sebastian Pop via llvm-commits]

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

Updated patch does not pass ninja check as it regresses in terms of number of hoisted instructions.
It may be because the MSSA still contains references to the BBs that used to contain ld/st and the dependence analysis is not able to move the remaining ld/st past these "ghost" ld/st that have been already hoisted.


https://reviews.llvm.org/D22966

Files:
  llvm/lib/Transforms/Scalar/GVNHoist.cpp

Index: llvm/lib/Transforms/Scalar/GVNHoist.cpp
===================================================================
--- llvm/lib/Transforms/Scalar/GVNHoist.cpp
+++ llvm/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.
@@ -310,7 +307,8 @@
 
     for (User *U : Def->users())
       if (auto *MU = dyn_cast<MemoryUse>(U)) {
-        BasicBlock *UBB = MU->getBlock();
+        // FIXME: MU->getBlock() does not get updated when we move the instruction.
+        BasicBlock *UBB = MU->getMemoryInst()->getParent();
         // Only analyze uses in BB.
         if (BB != UBB)
           continue;
@@ -725,6 +723,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 +742,10 @@
             !makeGepOperandsAvailable(Repl, HoistPt, InstructionsToHoist))
           continue;
 
+        // Move the instruction at the end of HoistPt.
         Repl->moveBefore(HoistPt->getTerminator());
+        if (MemoryAccess *MA = MSSA->getMemoryAccess(Repl))
+          NewMemAcc = dyn_cast<MemoryUseOrDef>(MA);
       }
 
       if (isa<LoadInst>(Repl))
@@ -775,6 +778,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.66187.patch
Type: text/x-patch
Size: 3129 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160729/0efe3d76/attachment.bin>