[PATCH] D63169: [GlobalISel][IRTranslator] Change switch table translation to generate jump tables and range checks.

[Jessica Paquette via Phabricator via llvm-commits]

paquette added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:413
+  if (!FuncInfo.BPI)
+    Src->addSuccessorWithoutProb(Dst);
+  else {
----------------
Just return after this to eliminate the else?


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:464-466
+    if (DefaultMBB != nextBlock(SwitchMBB)) {
+      MIB.buildBr(*DefaultMBB);
+    }
----------------
clang-format for braces


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:508-513
+MachineBasicBlock *IRTranslator::nextBlock(MachineBasicBlock *MBB) const {
+  MachineFunction::iterator I(MBB);
+  if (++I == MF->end())
+    return nullptr;
+  return &*I;
+}
----------------
Does this function have to exist at all? Can you just use `getNextNode`? 


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:551-567
+  if (!JTH.OmitRangeCheck) {
+    // Emit the range check for the jump table, and branch to the default block
+    // for the switch statement if the value being switched on exceeds the
+    // largest case in the switch.
+    auto Cst = getOrCreateVReg(
+        *ConstantInt::get(SValue.getType(), JTH.Last - JTH.First));
+    Cst = MIB.buildZExtOrTrunc(PtrScalarTy, Cst).getReg(0);
----------------
I think this can probably be simplified to reduce nesting, since the `if` case is much longer than the `else if` here.

E.g.

```
if (JTH.OmitRangeCheck && JT.MBB != nextBlock(SwitchBB)) {
    MIB.buildBr(*JT.MBB);
    return true;
}

auto Cst...
return true;
```


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:586-588
+    if (CB.TrueBB != nextBlock(CB.ThisBB)) {
+      MIB.buildBr(*CB.TrueBB);
+    }
----------------
clang-format


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:762
+  const TargetMachine &TM = CurMF->getTarget();
+  if (TM.getOptLevel() != CodeGenOpt::None) {
+    // Here, we order cases by probability so the most likely case will be
----------------
I think we should check the function attributes for optnone here as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63169