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

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: lib/CodeGen/BranchFolding.cpp:521
+static bool blockEndsInUnreachable(const MachineBasicBlock *MBB) {
+  return MBB->succ_empty() && !MBB->isReturnBlock();
+}
----------------
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.


https://reviews.llvm.org/D29153