[PATCH] D80863: [WebAssembly] Eliminate range checks on br_tables

[Thomas Lively via Phabricator via llvm-commits]

tlively added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyFixBrTableDefaults.cpp:45
+MachineBasicBlock *fixBrTable(MachineInstr &MI, MachineBasicBlock *MBB,
+                              MachineFunction &MF) {
+  // Get the header block, which contains the redundant range check.
----------------
aheejin wrote:
> Nit: It might help readers if there's a comment that prior to this function, `br_table` is missing the last (= default) operand, which will be added here
Done.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyFixBrTableDefaults.cpp:47
+  // Get the header block, which contains the redundant range check.
+  assert(std::next(MBB->pred_begin()) == MBB->pred_end());
+  auto *HeaderMBB = *MBB->pred_begin();
----------------
aheejin wrote:
> Nit: This might be more intuitive:
> ```
> assert(MBB->pred_size() == 1);
> ```
> 
> By the way, is it guaranteed that we always have a single predecessor that is a guard BB, which ends with a conditional branch when the default BB is reachable and no branch when it is unreachable? Are other patterns possible?
Done.

There is always a single guard BB that contains a conditional branch to the default target if the default target exists. Then the guard BB either falls through or has an unconditional jump to the jump table BB.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyFixBrTableDefaults.cpp:59
+    // Install the default target and remove the jumps in the header.
+    auto *DefaultMBB = JumpMII->getOperand(0).getMBB();
+    MI.addOperand(MF, MachineOperand::CreateMBB(DefaultMBB));
----------------
aheejin wrote:
> aheejin wrote:
> > Is this guaranteed that `br_table` BB is always a fallthrough, so the operand 0 is the default target? Can they be swapped?
> If it is somehow guaranteed, it is fine, but if we are unsure, we can compare `br_if`'s argument to `MBB` to figure it out.
They would only be swapped if something in between SelectionDAG building and this pass swapped them, but I'll add an assert in case some later change swaps the order.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyFixBrTableDefaults.cpp:64
+    // Arbitrarily choose the last jump target as the default.
+    auto *SomeMBB = MI.getOperand(MI.getNumExplicitOperands() - 1).getMBB();
+    MI.addOperand(MachineOperand::CreateMBB(SomeMBB));
----------------
aheejin wrote:
> Would there be performance implications based on what we choose arbitrarily for the default target, in case there's no guard hints? (I don't know and I'm just asking) If we are unsure, can it be safer to choose the first one, as we are doing now?
I suppose it might affect register allocation or something, but I'm not sure. I'll switch to using the first target.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyFixBrTableDefaults.cpp:75
+    if (HeaderMBB->isSuccessor(Succ))
+      HeaderMBB->removeSuccessor(Succ);
+  HeaderMBB->transferSuccessorsAndUpdatePHIs(MBB);
----------------
aheejin wrote:
> Why is this process (checking if `Succ` is a successor of `HeaderMBB` and deleting it from the successors) necessary?
We need to remove the shared successors first because otherwise `transferSuccessorsAndUpdatePHIs` could introduce duplicate successors, which is invalid. I'll add a comment to that effect.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyFixBrTableDefaults.cpp:119
+INITIALIZE_PASS(WebAssemblyFixBrTableDefaults, DEBUG_TYPE,
+                "Removes irreducible control flow", false, false);
+
----------------
sbc100 wrote:
> Lies!
Oops!


================
Comment at: llvm/test/CodeGen/WebAssembly/cfg-stackify.ll:652
 ; CHECK-NEXT:  block   {{$}}
-; CHECK:       br_if     0, {{[^,]+}}{{$}}
-; CHECK:       br        3{{$}}
-; CHECK-NEXT:  .LBB{{[0-9]+}}_7:
-; CHECK-NEXT:  end_block{{$}}
-; CHECK:       block   {{$}}
-; CHECK-NEXT:  br_table   $0, 0, 3, 1, 2, 0
-; CHECK-NEXT:  .LBB{{[0-9]+}}_8:
+; CHECK:       br_table   $pop{{[^,]+}}, 0, 3, 1, 2, 3
+; CHECK-NEXT:  .LBB{{[0-9]+}}_6:
----------------
aheejin wrote:
> sbc100 wrote:
> > Can this be CHECK-NEXT too now?  So we know the br_if is not there?
> Or we can explicitly check if it's not there, like
> ```
> CHECK-NOT: br_if ...
> ```
Used the `CHECK-NOT` because IIRC there's a bunch of extra junk in there preventing us from using `CHECK-NEXT`.


================
Comment at: llvm/test/CodeGen/WebAssembly/switch-unreachable-default.ll:7
+; Test that switches are lowered correctly in the presence of an
+; unreachable default branch target.
+
----------------
sbc100 wrote:
> So if the default label contains unreachable then we end up in some special case where that target doesn't actually exist in the final output, and we instead just arbitrarily jump to any other the labels?
> 
> Is this standard llvm behaviour?  Is is that default target in this case eliminated by some earlier pass?
Correct. The target-independent code doesn't emit any jump to a default target to begin with when the default is unreachable, so on other targets if the default did turn out to be reachable at runtime, then there would be an out-of-bounds access to the jump table.

The relevant code that creates the jump to the default block is here: https://github.com/llvm/llvm-project/blob/f027cfa37e6757bb2d17ac3bea944df4e06bcff4/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp#L2475-L2482.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80863