Thanks for looking! Comments addressed, the main ones being:

• restored the statistics, and
• the code that I forgot to delete.

Thanks, LGTM

Thanks guys, will address this before committing.

I think this should cover it now, and address the comments:

• RematerializeIterCount() is only called when we've cloned the VCTP instruction in an exit block, this makes the unrelated test case go away.
• RematerializeIterCount() has become really simple with just a call to formLCSSARecursively() (and of course rewriteLoopExitValues()) because when there's nothing to do, formLCSSARecursively is a cheap operation, so we indeed don't need to check first if the loop is in LCSSA form.
• One of the motivating examples for this work and optimisation is a reduction in vector-reduce-mve-tail.ll. I decided to use `formLCSSARecursively, instead of manually inserting a phi, because there are a few values live-out, the ones that feed the final reduction. We need to insert 3 Phi nodes, thus I don't think it is worth optimising and calling formLCSSARecursively is best.
• This is tested by exiting tests. For example, the reductions vector-reduce-mve-tail.ll will bring it in LCSSA form first, and is testing that. Simpler tests elsewhere check when this doesn't need to happen first.

I have removed WIP and TODO from the patch subject/description, as this revision now demonstrates I think what this patch should be doing:

• I have removed LoopPass from this, as this indeed causes a few more passes to run, which we don't need, and causes (unrelated) changes elsewhere. Now, the only test changes are in the LowOverheadLoop tests, which is what we want.
• I use formLCSSA() to transform the loop into LCSSA form if it isn't.

Thanks for sharing your thoughts!

Just added a comment for clarity saying that the IR function was intentionally left empty in the MIR test (the offending MIR sequence was extracted from a larger reproducer, and there's no need to copy the corresponding IR).

I guess this makes probably sense, but just checking why this should have the effect of enabling MVE-I? Is there prior art -mfpu=none has a similar effect? Has this been synchronised with the GCC community?
Do we need more tests, e.g. target parser unit tests, and some more negative tests fp16 isn't set or hard float abi?

A round of nits:

LGTM, with just one nit:

LGTM, with just a nit inlined.

Thanks, and comment addressed in rG07028b5a8780.

Thanks, and I will add that before committing.

I have added test cases for LFTR and loopunrolling showing that the said transformations don't trigger as int_loop_decrement_reg is described as IntrNoDuplicate

Just a quick message, linking in @samparker, and I guess moving Low Overhead Loops to run before ConstantIslands could be problematic, but we can/should have a proper look tomorrow.

Thanks for looking at this! And also for encouraging me to look at this (my own) spaghetti logic again. But to be fair, we have quite a few factors that play a role here: optimising for minsize takes precedence over the prefer predicate options, which take precedence over the loop hints, which take precedence over the TTI hook. I have explained this in the comments, and have reshuffled the logic accordingly. I am now bailing earlier on PredicateOptDisabled, as you suggested, loop hints need to be checked lastly, and thus this addresses your comments, I think.

Thanks, LGTM again

A last question/request, I guess we need 2 more tests? For completeness probably best to have a test with {{-enable-arm-maskedgatscat=false}} and another one with {{-mattr=-mve}}?

looks good to me too, other than the irrelevant nits, I have one question though, see inlined.

Thanks for taking a look, I have added test case mve-vpt-block-fold-vcmp.mir.

Good stuff!

Cheers, nice one, LGTM

Can you upload the patch with context?

Thanks again for sharing your thoughts and comments. I've done a bit of my homework:

Thanks for taking a look!

One more nit about adding comments. This is a nice summary / description:

Looks reasonable

Committed in 049f9672d8566f0d0a115f11e2a53018ea502b10
(I had a typo in the tag Diferential Revision, so this didn't get closed automatically)

Delayed deleting the VCMP, in order not to invalidate the RDA analysis.

Ah yes, thanks, I forgot about ARMBaseInstrInfo, but that's definitely the place where this should live.

Most of this is a test change!

Can we test this?

Yep, more RDA

Now using RDA

it's not that this is important, but the code would benefit from a rewrite. This was a more easy/straightforward change, but agreed that RDA would be even better, so will see what I can do.

Yep, that's a lot of changes... :-)
I haven't read or digested everything yet, so this a first round of nits.

Thanks for the reviews.

Thanks, comments addressed.

Ah, didn't see the LGTM until I pressed the button myself.

Looks like a good fix to me.

I'm not sure this is doing what you want.. wouldn't the vctp likely be a use before def..?

Currently this prevents us from compiling quite a few codes, so this LGTM while we sort things out.

Comments addressed and thanks for catching that problem, a test case has been added.

We've had this enabled by default for almost 1 year downstream now, which is a decent amount of time to shake out some codegen bugs. I agree that flipping the switch now make sense, and hopefully similar non-X86 targets enjoy decent performance improvements too.

Hi Sam, about the AArch64 results: I assume you targeted the A32 ISA, can you confirm that? Because that would mean we have 2 data points, at least 2 ISA for which this is beneficial: T32 and A32.

LGTM

Yep, nice combine

LGTM

Oh yes! Nice one!

Still LGTM

Thanks, and looks reasonable to me.

I wanted to look into this to make a less hand wavy suggestion, but I got lost a little bit lost, which always happens when I look in that jungle that is TTI and TTIImpl etc., so will just ask a question instead.

I don't think I have full context of the problem.... but in this diff I see type __fp16, which is the storage-only type. But if we are talking about v8.2-A and FP16, then we are talking about the native type (e.g. source-language type _Float16), and for that you need to enable the +fp16 architecture extension. I would have to refresh my memory, but I thought -fnative-half-type was used to enable OpenCL fp16 types? Either way, it looks like there's interaction between __fp16 and native FP16 types, but I don't think adding -fnative-half-type is the right approach. But as I said, I don't know exactly what the problem is, and would need to have a proper look when I'm back at my desk.