[PATCH] D108583: [WebAssembly] Use SSAUpdaterBulk in LowerEmscriptenSjLj

[Heejin Ahn via Phabricator via llvm-commits]

aheejin created this revision.
aheejin added reviewers: dschuff, tlively.
Herald added subscribers: wingo, ecnelises, sunfish, hiraditya, jgravelle-google, sbc100.
aheejin requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

We update SSA in two steps in Emscripten SjLj:

1. Rewrite uses of `setjmpTable` and `setjmpTableSize` variables and place `phi`s where necessary, which are updated where we call `saveSetjmp`.
2. Do a whole function level SSA update for all variables, because we split BBs where `setjmp` is called and there are possibly variable uses that are not dominated by a def. (See https://github.com/llvm/llvm-project/blob/955b91c19c00ed4c917559a5d66d14c669dde2e3/llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp#L1314-L1324)

We have been using `SSAUpdater` to do this, but `SSAUpdaterBulk` class
was added after this pass was first created, and for the step 2 it looks
like a better alternative with a possible performance benefit. Not sure
the author is aware of it, but `SSAUpdaterBulk` seems to have a
limitation: it cannot handle a use within the same BB as a def but
before it. For example:

  ... = %a + 1
  %a = foo();

or

  %a = %a + 1

The uses `%a` in RHS should be rewritten with another SSA variable of
`%a`, most likely one generated from a `phi`. But `SSAUpdaterBulk`
thinks all uses of `%a` are below the def of `%a` within the same BB.
(`SSAUpdater` has two different functions of rewriting because of this:
`RewriteUse` and `RewriteUseAfterInsertions`.) This doesn't affect our
usage in the step 2 because that deals with possibly non-dominated uses
by defs after block splitting. But it does in the step 1, which still
uses `SSAUpdater`.

But this CL also simplifies the step 1 by using `make_early_inc_range`,
removing the need to advance the iterator before rewriting a use.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D108583

Files:
  llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp
  llvm/test/CodeGen/WebAssembly/lower-em-sjlj.ll


Index: llvm/test/CodeGen/WebAssembly/lower-em-sjlj.ll
===================================================================
--- llvm/test/CodeGen/WebAssembly/lower-em-sjlj.ll
+++ llvm/test/CodeGen/WebAssembly/lower-em-sjlj.ll
@@ -142,7 +142,7 @@
 ; CHECK-NEXT: %[[SETJMP_TABLE_SIZE1:.*]] = call i32 @getTempRet0()
 
 ; CHECK: if.then.split:
-; CHECK: %[[VAR1:.*]] = phi i32 [ %[[VAR0]], %if.then ], [ %[[VAR2:.*]], %if.end3 ]
+; CHECK: %[[VAR1:.*]] = phi i32 [ %[[VAR2:.*]], %if.end3 ], [ %[[VAR0]], %if.then ]
 ; CHECK: %[[SETJMP_TABLE_SIZE2:.*]] = phi i32 [ %[[SETJMP_TABLE_SIZE1]], %if.then ], [ %[[SETJMP_TABLE_SIZE3:.*]], %if.end3 ]
 ; CHECK: %[[SETJMP_TABLE2:.*]] = phi i32* [ %[[SETJMP_TABLE1]], %if.then ], [ %[[SETJMP_TABLE3:.*]], %if.end3 ]
 ; CHECK: store i32 %[[VAR1]], i32* @global_var, align 4
@@ -222,7 +222,7 @@
   br label %for.cond
 
 ; CHECK: for.inc.split:
-  ; CHECK: %var[[VARNO]] = phi i32 [ %var, %for.inc ]
+  ; CHECK: %var[[VARNO]] = phi i32 [ undef, %if.end ], [ %var, %for.inc ]
 }
 
 ; Tests cases where longjmp function pointer is used in other ways than direct
Index: llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp
===================================================================
--- llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp
+++ llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp
@@ -200,6 +200,7 @@
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Transforms/Utils/BasicBlockUtils.h"
 #include "llvm/Transforms/Utils/SSAUpdater.h"
+#include "llvm/Transforms/Utils/SSAUpdaterBulk.h"
 
 using namespace llvm;
 
@@ -641,25 +642,23 @@
 void WebAssemblyLowerEmscriptenEHSjLj::rebuildSSA(Function &F) {
   DominatorTree &DT = getAnalysis<DominatorTreeWrapperPass>(F).getDomTree();
   DT.recalculate(F); // CFG has been changed
-  SSAUpdater SSA;
+  SSAUpdaterBulk SSA;
   for (BasicBlock &BB : F) {
     for (Instruction &I : BB) {
-      SSA.Initialize(I.getType(), I.getName());
-      SSA.AddAvailableValue(&BB, &I);
-      for (auto UI = I.use_begin(), UE = I.use_end(); UI != UE;) {
-        Use &U = *UI;
-        ++UI;
+      unsigned VarID = SSA.AddVariable(I.getName(), I.getType());
+      SSA.AddAvailableValue(VarID, &BB, &I);
+      for (auto &U : I.uses()) {
         auto *User = cast<Instruction>(U.getUser());
         if (auto *UserPN = dyn_cast<PHINode>(User))
           if (UserPN->getIncomingBlock(U) == &BB)
             continue;
-
         if (DT.dominates(&I, User))
           continue;
-        SSA.RewriteUseAfterInsertions(U);
+        SSA.AddUse(VarID, &U);
       }
     }
   }
+  SSA.RewriteAllUses(&DT);
 }
 
 // Replace uses of longjmp with emscripten_longjmp. emscripten_longjmp takes
@@ -1301,24 +1300,14 @@
   for (Instruction *I : SetjmpTableSizeInsts)
     SetjmpTableSizeSSA.AddAvailableValue(I->getParent(), I);
 
-  for (auto UI = SetjmpTable->use_begin(), UE = SetjmpTable->use_end();
-       UI != UE;) {
-    // Grab the use before incrementing the iterator.
-    Use &U = *UI;
-    // Increment the iterator before removing the use from the list.
-    ++UI;
+  for (auto &U : make_early_inc_range(SetjmpTable->uses()))
     if (auto *I = dyn_cast<Instruction>(U.getUser()))
       if (I->getParent() != Entry)
         SetjmpTableSSA.RewriteUse(U);
-  }
-  for (auto UI = SetjmpTableSize->use_begin(), UE = SetjmpTableSize->use_end();
-       UI != UE;) {
-    Use &U = *UI;
-    ++UI;
+  for (auto &U : make_early_inc_range(SetjmpTableSize->uses()))
     if (auto *I = dyn_cast<Instruction>(U.getUser()))
       if (I->getParent() != Entry)
         SetjmpTableSizeSSA.RewriteUse(U);
-  }
 
   // Finally, our modifications to the cfg can break dominance of SSA variables.
   // For example, in this code,


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D108583.368206.patch
Type: text/x-patch
Size: 3743 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210823/c7759ded/attachment.bin>