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

[Amara Emerson via Phabricator via llvm-commits]

aemerson marked 13 inline comments as done.
aemerson added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/IRTranslator.h:505
+  public:
+    GISelSwitchLowering(IRTranslator *irt, FunctionLoweringInfo &funcinfo)
+      : SwitchLowering(funcinfo), IRT(irt) {}
----------------
paquette wrote:
> 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?
It's a deliberate choice for me. I'm not sure if it's correct but there's some precedent for this around the codebase. I prefer in cases where we have identically named member variables and parameters to just use lower case for the very short lived parameters.

It won't be null since it's used in one place, I'll put an assert anyway.


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


================
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");
----------------
paquette wrote:
> Is `*I.getCaseSuccessor()` always guaranteed to not be `nullptr`?
Yes I think so.


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:433-435
+    BranchProbability Prob =
+        BPI ? BPI->getEdgeProbability(SI.getParent(), I.getSuccessorIndex())
+            : BranchProbability(1, SI.getNumCases() + 1);
----------------
paquette wrote:
> Could this use the above `IRTranslator::getEdgeProbability` function? Then you don't have to check `BPI` here.
I don't think it computes exactly the same thing, SI.getNumCases() + 1 can't be replaced because the successors of SI's parent BB is not necessarily the same as the successors of the switchinst itself.


================
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
----------------
paquette wrote:
> This (and the remainder of this function) seems like it could be factored out into a `clusterAdjacentCases` helper function.
Not sure what you mean here, the clustering is already done in this call to sortAndRangeify(). The rest of the function is unrelated.


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


================
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);
----------------
paquette wrote:
> Is it worth pulling JTH.SValue out into a variable?
Ok.


================
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) {
----------------
paquette wrote:
> `llvm::stable_sort`?
I don't think the Low values can overlap, so we don't need stable sort here.


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


================
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;
+      }
----------------
paquette wrote:
> remove braces to appease llvm style gods
Ok.


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:777-778
+      break;
+    }
+    }
+    CurMBB = Fallthrough;
----------------
paquette wrote:
> brace formatting seems wonky here
Because switch cases aren't indented relative to the switch.


================
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) {
----------------
paquette wrote:
> Just curious, why 16?
Pointer storage is cheap, and set growth is expensive, just a rough guess.


================
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);
----------------
paquette wrote:
> capital `i` to appease the llvm style gods, but I'll pretend I didn't see this ;)
Ok.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63169