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

[Phillip Raffeck via Phabricator via llvm-commits]

praffeck added inline comments.


================
Comment at: lib/Target/ARM/ARMConstantIslandPass.cpp:456
   if (GenerateTBB && !STI->genExecuteOnly())
     MadeChange |= optimizeThumb2JumpTables();
 
----------------
efriedma wrote:
> 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.
Yeah, applying these shrinking checks in a loop seems sensible.

Regarding bl vs b.w: Should I include it in this patch? Or would it be better to introduce a separate patch for replacing unnecessary bl and b.w instructions?


================
Comment at: lib/Target/ARM/ARMConstantIslandPass.cpp:458
 
+  while (true) {
+    bool BRChange = false;
----------------
efriedma wrote:
> Maybe add a one-line comment explaining what this loop is doing?
> 
> Maybe put this inside an `if (HasFarJump())`?
Ok, will do.


================
Comment at: lib/Target/ARM/ARMConstantIslandPass.cpp:1638
+
+  // FIXME: Only replacing unconditional branches for now.
+  if (Br.isCond)
----------------
efriedma wrote:
> tBfar is always unconditional.
Of course it is, my bad. Will remove that.


================
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);
----------------
efriedma wrote:
> tBfar is only used in Thumb1 mode.
Again, should b.w also be replaced and if yes, should that happen in a different patch?


================
Comment at: lib/Target/ARM/ARMConstantIslandPass.cpp:2293
+    int TableDelta = OrigTableSize - NewTableSize;
+    BBInfo[TableMBB->getNumber()].Size -= TableDelta;
+    adjustBBOffsetsAfter(TableMBB);
----------------
efriedma wrote:
> 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?)
It should always be the only MI, yeah. I will fix it.


Repository:
  rL LLVM

https://reviews.llvm.org/D50800