[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/include/llvm/CodeGen/GlobalISel/IRTranslator.h:505
+  public:
+    GISelSwitchLowering(IRTranslator *irt, FunctionLoweringInfo &funcinfo)
+      : SwitchLowering(funcinfo), IRT(irt) {}
----------------
I think you can capitalize `irt` etc. in here

Also, can `irt` ever be null (I doubt it, but still)? Should there be an assert?


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:429
+  Clusters.reserve(SI.getNumCases());
+  for (auto I : SI.cases()) {
+    MachineBasicBlock *Succ = &getMBB(*I.getCaseSuccessor());
----------------
`for (auto &I : ... )`?


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:430
+  for (auto I : SI.cases()) {
+    MachineBasicBlock *Succ = &getMBB(*I.getCaseSuccessor());
+    assert(Succ && "Could not find successor mbb in mapping");
----------------
Is `*I.getCaseSuccessor()` always guaranteed to not be `nullptr`?


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:433-435
+    BranchProbability Prob =
+        BPI ? BPI->getEdgeProbability(SI.getParent(), I.getSuccessorIndex())
+            : BranchProbability(1, SI.getNumCases() + 1);
----------------
Could this use the above `IRTranslator::getEdgeProbability` function? Then you don't have to check `BPI` here.


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:441
+
+  // Cluster adjacent cases with the same destination. We do this at all
+  // optimization levels because it's cheap to do and will make codegen faster
----------------
This (and the remainder of this function) seems like it could be factored out into a `clusterAdjacentCases` helper function.


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:495
+
+MachineBasicBlock *IRTranslator::NextBlock(MachineBasicBlock *MBB) const {
+  MachineFunction::iterator I(MBB);
----------------
to appease the llvm style gods, this should be `nextBlock`


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:524
+  // Subtract the lowest switch case value from the value being switched on.
+  const LLT SwitchTy = getLLTForType(*JTH.SValue->getType(), *DL);
+  unsigned SwitchOpReg = getOrCreateVReg(*JTH.SValue);
----------------
Is it worth pulling JTH.SValue out into a variable?


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:650
+    // as a tie-breaker as clusters are guaranteed to never overlap.
+    llvm::sort(W.FirstCluster, W.LastCluster + 1,
+               [](const CaseCluster &a, const CaseCluster &b) {
----------------
`llvm::stable_sort`?


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:659-668
+    for (CaseClusterIt I = W.LastCluster; I > W.FirstCluster;) {
+      --I;
+      if (I->Prob > W.LastCluster->Prob)
+        break;
+      if (I->Kind == CC_Range && I->MBB == NextMBB) {
+        std::swap(*I, *W.LastCluster);
+        break;
----------------
I feel like this is somewhat unintuitive. Is there a way to simplify this using some STL magic?


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:691
+
+    switch (I->Kind) {
+    case CC_BitTests:
----------------
This switch makes this function rather large. Would it make sense to factor it out into a helper function?


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:693
+    case CC_BitTests:
+      return false; // Bit tests currently unimplemented.
+    case CC_JumpTable: {
----------------
Debug output would be nice here


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:729-732
+      if (FallthroughUnreachable) {
+        // Skip the range check if the fallthrough block is unreachable.
+        JTH->OmitRangeCheck = true;
+      }
----------------
remove braces to appease llvm style gods


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:777-778
+      break;
+    }
+    }
+    CurMBB = Fallthrough;
----------------
brace formatting seems wonky here


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:2024
 
-    // All MachineBasicBlocks exist, add them to the PHI. We assume IRTranslator
-    // won't create extra control flow here, otherwise we need to find the
-    // dominating predecessor here (or perhaps force the weirder IRTranslators
-    // to provide a simple boundary).
-    SmallSet<const BasicBlock *, 4> HandledPreds;
-
+    SmallSet<const MachineBasicBlock *, 16> SeenPreds;
     for (unsigned i = 0; i < PI->getNumIncomingValues(); ++i) {
----------------
Just curious, why 16?


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:2129
+void IRTranslator::finalizeBasicBlock() {
+  for (unsigned i = 0, e = SL->JTCases.size(); i != e; ++i)
+    emitJumpTable(SL->JTCases[i].second, SL->JTCases[i].second.MBB);
----------------
capital `i` to appease the llvm style gods, but I'll pretend I didn't see this ;)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63169