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

[Derek Schuff via Phabricator via llvm-commits]

dschuff added a comment.

LGTM With those changes.



================
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())
----------------
aheejin wrote:
> sunfish wrote:
> > 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.
> @dschuff Let me clarify a bit. You can think of those two queue as just queue Alpha and queue Beta or something. Here they happen to be `CompareBlockNumbersForward` and `CompareBlockNumbersBackward` and they sound like they have some opposite relationship and thus confusing, but they are just different ways to pick a block. What I want is, I want to impose a universal rule of "pick an EH pad first whenever it is available" for all queues available. So why the queues sort in the opposite direction doesn't really matter. And also having a separate queue for EH pads wouldn't do what I want (I want impose a universal rule for all queues, as I said). If we make this as a separate queue, it is unclear how we should modify the logic itself to figure out when we should switch to this specific queue.
> 
> For comment-wise, I'm not sure what I should add more to what's already there. Should I add what I described here?
Maybe the long comment about EH pad handling could go outside both comparators, and it could note that EH pads are selected first regardless of the block comparison order. I also actually think that changing the name to `CompareBlockNumbersAndEHPad` doesn't really make them more understandable, and I'd probably just leave the names the same (while keeping the comments).


================
Comment at: lib/Target/WebAssembly/WebAssemblyCFGSort.cpp:252
   // processed successors, to help preserve block sequences from the original
   // order. Ready has the remaining ready blocks.
   PriorityQueue<MachineBasicBlock *, std::vector<MachineBasicBlock *>,
----------------
Could also add one note here like  'EH blocks are picked first from both queues.'


Repository:
  rL LLVM

https://reviews.llvm.org/D46500