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

[Alon Zakai via Phabricator via llvm-commits]

kripken marked 17 inline comments as done.
kripken added a comment.

Thanks, submitting a fixed patch now.



================
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
----------------
aheejin wrote:
> assign to -> assign?
We assign to the same one in all paths. I'll reword it.


================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:183
+    }
   }
 
----------------
aheejin wrote:
> 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.
To make sure I understand, inlining the remaining contents would mean doing the same work 3 times (for each inlined location) to do the insert, check if it actually added, and add to the work list if so? That feels less good to me, but happy to do it either way.


================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:222
+  SmallPtrSet<MachineBasicBlock *, 4> Entries;
+  SmallVector<MachineBasicBlock *, 4> SortedEntries;
+  for (auto *Looper : Loopers) {
----------------
aheejin wrote:
> 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?
Hmm, can I sort a SetVector, though? It says the order of iteration is the order of insertion (which is fixed).


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

https://reviews.llvm.org/D55467