[PATCH] D29153: [BranchFolding] Tail common all identical unreachable blocks

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

Aside from the possible issue with `isReturnBlock`, are there any comments on the patch functionality?



================
Comment at: lib/CodeGen/BranchFolding.cpp:521
+static bool blockEndsInUnreachable(const MachineBasicBlock *MBB) {
+  return MBB->succ_empty() && !MBB->isReturnBlock();
+}
----------------
sdardis wrote:
> rengolin wrote:
> > iteratee wrote:
> > > rnk wrote:
> > > > rengolin wrote:
> > > > > iteratee wrote:
> > > > > > rnk wrote:
> > > > > > > iteratee wrote:
> > > > > > > > Unforturnately there are platforms where this isn't sufficient. I believe ARM is one of them.
> > > > > > > For all the ARM examples I constructed, isReturnBlock appeared to be accurate.
> > > > > > OK, I have data.
> > > > > > 
> > > > > > isReturnBlock is currently insufficient for Thumb (That's why I thought ARM), Hexagon, Mips and XCore.
> > > > > > Arguably, this should go in anyway and those platforms should be fixed.
> > > > > > 
> > > > > > Funnily enough, this could cause switching away from Thumb to reduce code size.
> > > > > Can you be more specific for "insufficient"?
> > > > > 
> > > > > What's the risk/cost? If they're too big, than "fixing it later" might not be a good idea.
> > > > > 
> > > > > Also, who's fixing it?
> > > > > Can you be more specific for "insufficient"?
> > > > 
> > > > It returns false on blocks that end in returns. Grepping through ARMThumbInstrInfo suggests that some but not all returns have the isReturn flag set, so this probably only affects some small number of returns.
> > > > 
> > > > > What's the risk/cost? If they're too big, than "fixing it later" might not be a good idea.
> > > > 
> > > > This change makes us tail merge small identical noreturn blocks (zero successors without a return). The risk is that we tail merge a return block that was duplicated to get better code layout. In practice, I have found it very hard to show a functional difference. The ARM backend will do things like predicating entire small return blocks, making it really hard to construct a test case if you don't know both the ARM ISA and the ARM backend options well.
> > > > 
> > > > > Also, who's fixing it?
> > > > 
> > > > I wasn't going to. The risk is really low. I doubt this transform will ever fire on important code for ARM.
> > > Those platforms don't correctly mark return instructions as returns.
> > > 
> > > Given that this is a size decrease on all platforms where return instructions are correctly marked, I see no problem with it going in as is.
> > > 
> > > The platform owners should fix it if they care.
> > > I wasn't going to. The risk is really low. I doubt this transform will ever fire on important code for ARM.
> > 
> > Can you add a FIXME to alert the respective back-end owners that their targets are (possibly) negatively affected by the change?
> > 
> > > The platform owners should fix it if they care.
> > 
> > This is **extremely** selfish.
> > 
> > If you find a problem on some architecture that is not your primary, the **least** you can do is to contact them and get their reviews.
> > 
> > I'm usually responsive and pro-active (as this reply is demonstration), so you can't even use the argument that you tried in the past and failed.
> > 
> > I'm not sure how you came to this conclusion, but I wish you'd be more careful next time.
> > isReturnBlock is currently insufficient for Thumb (That's why I thought ARM), Hexagon, Mips and XCore.
> > Arguably, this should go in anyway and those platforms should be fixed.
> 
> Can you file a bug report on this, CCing the code owners of the affected targets?
I'm not sure what issue Kyle is seeing. It looks like we have return instructions for ARM, thumb1, and thumb2, so isReturnBlock should work properly:
```
$ git grep isReturn ../lib/Target/ARM/*.td
../lib/Target/ARM/ARMCallingConv.td:def CSR_AAPCS_ThisReturn : CalleeSavedRegs<(add LR, R11, R10, R9, R8, R7, R6,
../lib/Target/ARM/ARMCallingConv.td:def CSR_iOS_ThisReturn : CalleeSavedRegs<(add LR, R7, R6, R5, R4,
../lib/Target/ARM/ARMCallingConv.td:                                         (sub CSR_AAPCS_ThisReturn, R9))>;
../lib/Target/ARM/ARMInstrInfo.td:let isReturn = 1, isTerminator = 1, isBarrier = 1 in {
../lib/Target/ARM/ARMInstrInfo.td:let isCall = 1, isTerminator = 1, isReturn = 1, isBarrier = 1, Uses = [SP] in {
../lib/Target/ARM/ARMInstrInfo.td:let isReturn = 1, isBarrier = 1, isTerminator = 1, Defs = [PC] in
../lib/Target/ARM/ARMInstrInfo.td:let isReturn = 1, isTerminator = 1, isBarrier = 1, mayLoad = 1,
../lib/Target/ARM/ARMInstrThumb.td:let isReturn = 1, isTerminator = 1, isBarrier = 1 in {
../lib/Target/ARM/ARMInstrThumb.td:let isCall = 1, isTerminator = 1, isReturn = 1, isBarrier = 1 in {
../lib/Target/ARM/ARMInstrThumb.td:let isReturn = 1, isTerminator = 1, isBarrier = 1, mayLoad = 1,
../lib/Target/ARM/ARMInstrThumb2.td:let isReturn = 1, isTerminator = 1, isBarrier = 1, mayLoad = 1,
../lib/Target/ARM/ARMInstrThumb2.td:let isCall = 1, isTerminator = 1, isReturn = 1, isBarrier = 1 in {
../lib/Target/ARM/ARMInstrThumb2.td:let isReturn = 1, isBarrier = 1, isTerminator = 1, Defs = [PC] in
../lib/Target/ARM/ARMInstrThumb2.td:let isReturn = 1, isBarrier = 1, isTerminator = 1, Defs = [PC] in
```


https://reviews.llvm.org/D29153