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

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added a comment.

I didn't know about this optimization opportunity! Please make sure to run emscripten tests before landing because it contains some new assumptions.



================
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.
----------------
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


================
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();
----------------
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?


================
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));
----------------
Is this guaranteed that `br_table` BB is always a fallthrough, so the operand 0 is the default target? Can they be swapped?


================
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));
----------------
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?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyFixBrTableDefaults.cpp:75
+    if (HeaderMBB->isSuccessor(Succ))
+      HeaderMBB->removeSuccessor(Succ);
+  HeaderMBB->transferSuccessorsAndUpdatePHIs(MBB);
----------------
Why is this process (checking if `Succ` is a successor of `HeaderMBB` and deleting it from the successors) necessary?


================
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:
----------------
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 ...
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80863