[PATCH] D46500: [WebAssembly] CFG sort support for exception handling

[Dan Gohman via Phabricator via llvm-commits]

sunfish accepted this revision.
sunfish added inline comments.


================
Comment at: lib/Target/WebAssembly/WebAssemblyCFGSort.cpp:14
 /// This pass reorders the blocks in a function to put them into topological
-/// order, ignoring loop backedges, and without any loop being interrupted
-/// by a block not dominated by the loop header, with special care to keep the
-/// order as similar as possible to the original order.
+/// order, ignoring loop backedges, and without any loop and exception being
+/// interrupted by a block not dominated by the its header, with special care
----------------
This "and" should be "or".


================
Comment at: lib/Target/WebAssembly/WebAssemblyCFGSort.cpp:70
+
+class SortUnitInfo {
+  const MachineLoopInfo &MLI;
----------------
Could you add a comment for this class that briefly describes its purpose? Do I understand correctly that this is essentially the SortUnit analog of what LoopInfo is for loops?


================
Comment at: lib/Target/WebAssembly/WebAssemblyCFGSort.cpp:200
                   const MachineBasicBlock *B) const {
+    // We give a higher priority to an EH pad
+    if (A->isEHPad() && !B->isEHPad())
----------------
dschuff wrote:
> aheejin wrote:
> > dschuff wrote:
> > > Even when comparing backwards? I don't quite understand why this shouldn't just be the inverse of forward comparison.
> > For some reason I don't fully understand, for `Ready` list later BBs are selected first. But my intention here is no matter whether the list is `Preferred` (which uses `CompareBlockNumbers`) or `Ready` (which uses `CompareBlockNumbersBackwards`), whenever we have an EH pad ready, we want to pick it first.
> > 
> > So `CompareBlockNumbersBackwards` doesn't mean its results will be used in the reverse direction. Regardless of the comparison function, a BB indicated by the function as having higher priority is placed before when sorting.
> OK. This whole bit with the comparison functions still seems a little awkward to me though. At the very least there should be a comment here saying something about the usage or why it's not just the inverse of the above (especially since the name seems to imply that it is). Or maybe it just needs a better name?
> 
> Perhaps @sunfish knows why the queues sort in the opposite directions.
> 
> Or, stepping back, more, would it make more sense to just have separate queue (alongside Preferred and Ready) for EH pads? Preferred and Ready are basically 2 different priority classes, and we are now adding a third, but we are handling its prioritization differently from the other 2.
It was an early attempt at balancing two concerns: preserving the original code layout to the extent possible, while also keeping contiguous sequences contiguous. It worked on some simple testcases. It's probably not optimal.


Repository:
  rL LLVM

https://reviews.llvm.org/D46500