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

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added inline comments.


================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:183
+    }
   }
 
----------------
kripken wrote:
> 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.
I thought, after taking out calling `canonicalizeSuccessor` out of `maybeInsert`, `maybeInsert` would be like 2-3 lines, so it would be OK to inline, but maybe it's less desirable. I was mainly concerned about calling `canonicalize` and `canonicalizeSuccessor` in two different places, but maybe it's ok this way. :)


================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:222
+  SmallPtrSet<MachineBasicBlock *, 4> Entries;
+  SmallVector<MachineBasicBlock *, 4> SortedEntries;
+  for (auto *Looper : Loopers) {
----------------
kripken wrote:
> 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).
If the insertion / iteration order is fixed, do we need sorting at all?


================
Comment at: test/CodeGen/WebAssembly/irreducible-cfg-exceptions.ll:49
+
+_ZNSt3__215basic_streambufIcNS_11char_traitsIcEEE5sgetcEv.exit.i19.i.i: ; preds = %_ZNKSt3__219istreambuf_iteratorIcNS_11char_traitsIcEEE14__test_for_eofEv.exit.i.i
+  invoke void undef()
----------------
I understand this test is complex, but maybe for these couple extremely long basic block names, we can simplify them a bit..?


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

https://reviews.llvm.org/D55467