[PATCH] D55467: [WebAssembly] Optimize Irreducible Control Flow

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added a comment.

In the CL/commit description, could you add some explanation on what has changed, i.e., what unnecessary cases of transformation this patch reduces?



================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:85
     AU.addRequired<MachineDominatorTree>();
     AU.addPreserved<MachineDominatorTree>();
     AU.addRequired<MachineLoopInfo>();
----------------
kripken wrote:
> aheejin wrote:
> > I don't think we need dominator tree anymore, so I think we can delete these two lines.
> You're right that we don't need it, but it turns out recomputing MLI crashes if I don't keep this code there. I guess the dominator tree is used in MLI, and MLI does not know to recompute it itself? (I don't know anything about LLVM internals...)
Oh, you're right, sorry.


================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:24
+/// then we have irreducible control flow. We fix that as follows: in each
+/// block reaching an entry we assign to a "label" helper variable with an id
+/// of the block we wish to reach, and branch to a new block that dispatches to
----------------
assign to -> assign?


================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:76
+  LoopFixer(MachineFunction &MF, MachineLoopInfo &MLI, MachineLoop *Loop) :
+    MF(MF), MLI(MLI), Loop(Loop) {}
+
----------------
clang-format


================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:98
+  // Get a canonical block to represent a block or a loop: the block, or if in
+  // a loop, the loop header, of it in an outer loop scope, we can ignore it.
+  MachineBasicBlock *canonicalize(MachineBasicBlock *MBB) {
----------------
a loop -> an inner loop, just to be clear


================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:183
+    }
   }
 
----------------
Just a suggestion on the workflow... Currently `WorkList` is a class variable modified in multiple functions (inserted in `maybeInsert` and popped in `run`), and `maybeInsert` takes `MBB` and `Succ` but `MBB` is already canonicalized at that point but it canonicalize `Succ` within `maybeInsert` and if it is not canonicalized we bail out, which makes the workflow little hard to follow.

I think making `WorkList` as just a local variable within `run` and call both `canonicalize` and `canonicalizeSuccessor` in `run`, and inlining the remaining contents of `maybeInsert` into the `run` function would make it a bit simpler.


================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:222
+  SmallPtrSet<MachineBasicBlock *, 4> Entries;
+  SmallVector<MachineBasicBlock *, 4> SortedEntries;
+  for (auto *Looper : Loopers) {
----------------
Do we need two separate containers? If you want a vector with a O(1) search, maybe you can use `SetVector` and merge these two?


================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:235
+  // Check if we found irreducible control flow.
+  if (Entries.size() <= 1)
     return false;
----------------
How about `if (LLVM_LIKELY(Entries.size() <= 1))` to signal we wouldn't find an irreducible control flow in usual cases?


================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:280
+
+  // Rewrite the problematic successors for every that wants to reach the bad
+  // blocks. For simplicity, we just introduce a new block for every edge we
----------------
Is there a word missing between 'every' and 'that'?


================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:397
+  // here.
+  while (runIteration(MF, MLI)) {
+    // We rewrote part of the function; recompute MLI and start again.
----------------
How about `while (LLVM_UNLIKELY(runIteration(MF, MLI))` to signal that this is unlikely to be true?


================
Comment at: test/CodeGen/WebAssembly/irreducible-cfg-exceptions.ll:4
+target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
+target triple = "wasm32-unknown-unknown-wasm"
+
----------------
Now we use just "wasm32-unknown-unknown". In other test cases too.


================
Comment at: test/CodeGen/WebAssembly/irreducible-cfg-exceptions.ll:6
+
+$crashy = comdat any
+
----------------
Can we delete this?


================
Comment at: test/CodeGen/WebAssembly/irreducible-cfg-exceptions.ll:17
+; Function Attrs: minsize noinline optsize
+define hidden void @crashy() unnamed_addr #0 comdat personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
+entry:
----------------
Can we delete all attributes (like #0, #1, ...) from this and other test cases if they are not necessary to show the bug? Also `hidden` and `unnamed_addr` can be deleted as well.


================
Comment at: test/CodeGen/WebAssembly/irreducible-cfg-exceptions.ll:113
+attributes #1 = { minsize optsize }
+attributes #2 = { minsize nounwind optsize }
+
----------------
We can delete these if not necessary (for other test cases too)


================
Comment at: test/CodeGen/WebAssembly/irreducible-cfg-nested.ll:11
+
+define dso_local fastcc void @tre_parse() unnamed_addr {
+entry:
----------------
I think we can delete `dso_local`, `fastcc`, and `unnamed_addr` here


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55467/new/

https://reviews.llvm.org/D55467