[PATCH] D85233: [GlobalISel] Implement bit-test switch table optimization

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:723
+                                     MachineBasicBlock *SwitchBB) {
+  MachineIRBuilder MIB(*SwitchBB->getParent());
+  MIB.setMBB(*SwitchBB);
----------------
Should just change the insert point for the existing one? This loses the CSEInfo


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:732
+  Register MinValReg = MIB.buildConstant(SwitchOpTy, B.First).getReg(0);
+  auto RangeSub = MIB.buildSub({SwitchOpTy}, SwitchOpReg, MinValReg);
+
----------------
Don't need the {}


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:738
+  bool UsePtrType = false;
+  if (!TLI.isTypeLegal(SwitchVT)) {
+    UsePtrType = true;
----------------
Why depend on "legal" types? The concept should mostly be going away


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:751
+  if (UsePtrType) {
+    SwitchVT = TLI.getPointerTy(*DL);
+    SubReg = MIB.buildZExtOrTrunc(getLLTForMVT(SwitchVT), SubReg).getReg(0);
----------------
This loses the address space, and I'm not sure why you would ever be avoiding the pointer type. Also, in general you should never need to call getPointerTy, since you can just re-use the type of the thing you already have


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:770
+                                  RangeSub, RangeCst);
+    MIB.buildBrCond(RangeCmp.getReg(0), *B.Default);
+  }
----------------
Don't need getReg


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:784
+                                    MachineBasicBlock *SwitchBB) {
+  MachineIRBuilder MIB(*SwitchBB->getParent());
+  MIB.setMBB(*SwitchBB);
----------------
Same as above


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:789
+  LLT SwitchTy = getLLTForMVT(BB.RegVT);
+  Register Cmp = 0;
+  unsigned PopCount = countPopulation(B.Mask);
----------------
Don't need = 0


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:943
+                                FallthroughUnreachable)) {
+        LLVM_DEBUG(dbgs() << "Failed to lower bit test for switch");
+        return false;
----------------
Missing newline


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85233