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

[Alon Zakai via Phabricator via llvm-commits]

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

Thanks for the comments, uploading an updated patch now.



================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:183
+    }
   }
 
----------------
aheejin wrote:
> 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. :)
I'll try to improve the comments to clarify that. There should be just one location that calls canonicalizeSuccessor (maybeInsert), aside from an assertion. I think the assertions help, but if you feel they make the code less clear, then that's fine with me of course, and it would be shorter with fewer of them.

I'll also add some comments on where canonicalize/canonicalizeSuccessor should be called, so hopefully they look unsurprising in those locations.


================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:222
+  SmallPtrSet<MachineBasicBlock *, 4> Entries;
+  SmallVector<MachineBasicBlock *, 4> SortedEntries;
+  for (auto *Looper : Loopers) {
----------------
aheejin wrote:
> 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?
Yeah, good point - I was using an unsorted set earlier on (for LoopBlocks), but if I replace it with a SetVector too then everything indeed ends up deterministic.

I do have some vague worry about using SetVectors everywhere adding some overhead, but probably I'm overthinking it?


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

https://reviews.llvm.org/D55467