[PATCH] D71992: [ARM] Unrestrict Armv8 IT blocks

Wed Jan 15 13:29:08 PST 2020

efriedma added a comment.

I'm not really tracking performance for 32-bit armv8-a at the moment... but I would be fine with changing the default with appropriate testing.  I don't think we should have core-specific heuristics unless there's evidence some core is actually different in a meaningful way.

I'm a little concerned this could have some unexpected impact; getting rid of extra "it" markers probably doesn't have any performance impact, but more aggressive predication of 32-bit instructions could.  (I would guess 32-bit instructions that have an equivalent 16-bit instruction are fine, but it might not be wise to predicate certain expensive instructions, like udiv.)

================
Comment at: llvm/lib/Target/ARM/ARMFeatures.h:77
 // there are some "conditionally deprecated" opcodes
   case ARM::tADDspr:
   case ARM::tBLXr:
----------------
samparker wrote:
> efriedma wrote:
> > The checks for tADDspr and tADDrSP look wrong; neither of those instructions are deprecated, as far as I can tell. Maybe it was confused with something else?  (I guess this isn't really part of you patch; feel free to ignore.)
> > 
> > Are these checks intentionally not controlled by the "Restricted" boolean?
> There's a note in Table F1-15 'Non-deprecated IT 16-bit conditional instructions' about using the PC, I think this logic is okay. And yes, it's intention, I've heard that some cores branch predictors don't work as well when some of these instructions are used.
I was going to ask about equivalent 32-bit instructions, but I guess most Thumb2 instructions aren't allowed to access pc anyway.

If you're worried about performance penalties for certain branches, can we handle that in a separate "isWeirdBranch" function?  It sounds like the concern here is independent of the general deprecation.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71992/new/

https://reviews.llvm.org/D71992