[PATCH] D50800: [ARM] Use correct jump table sizes

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: lib/Target/ARM/ARMConstantIslandPass.cpp:456
   if (GenerateTBB && !STI->genExecuteOnly())
     MadeChange |= optimizeThumb2JumpTables();
 
----------------
I wonder if we should rearrange these shrinking checks... or maybe make it a loop.  As-is, this patch only shrinks Thumb1 "far" (bl) branches based on jump tables, and not the Thumb2 b.w.


================
Comment at: lib/Target/ARM/ARMConstantIslandPass.cpp:458
 
+  while (true) {
+    bool BRChange = false;
----------------
Maybe add a one-line comment explaining what this loop is doing?

Maybe put this inside an `if (HasFarJump())`?


================
Comment at: lib/Target/ARM/ARMConstantIslandPass.cpp:1638
+
+  // FIXME: Only replacing unconditional branches for now.
+  if (Br.isCond)
----------------
tBfar is always unconditional.


================
Comment at: lib/Target/ARM/ARMConstantIslandPass.cpp:1643
+  MachineBasicBlock *DestBB = MI->getOperand(0).getMBB();
+  unsigned NearBrOpc = isThumb ? (isThumb2 ? ARM::t2B : ARM::tB) : ARM::B;
+  unsigned MaxNearDisp = getUnconditionalBrDisp(NearBrOpc);
----------------
tBfar is only used in Thumb1 mode.


================
Comment at: lib/Target/ARM/ARMConstantIslandPass.cpp:2293
+    int TableDelta = OrigTableSize - NewTableSize;
+    BBInfo[TableMBB->getNumber()].Size -= TableDelta;
+    adjustBBOffsetsAfter(TableMBB);
----------------
Not sure `Size -= TableDelta` is correct... or at least, it's confusing.

If the jump-table is the only MI in the block, then you could just write `BBInfo[TableMBB->getNumber()].Size` = NewTableSize`.  If it isn't, you're probably not handling alignment correctly.  (IIRC it's always the only MI?)


Repository:
  rL LLVM

https://reviews.llvm.org/D50800