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

[Renato Golin via Phabricator via llvm-commits]

rengolin added inline comments.


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


https://reviews.llvm.org/D29153