Ah, I now see that Dave commented on the other ticket, that probably needs to be addressed first...

LGTM (perhaps the same nit as in the other review)

LGTM, with one nit that doesn't need another review.

Rewrite using new function reverseInductionVariable in LoopInfo, see also the updated Title/Summary.

Ah yes, it's getCallCost calling getIntrinsicCost, and this is about getCallCost, so agreed.

This is a tricky one, because usually it is quite simpel: if code is not used, it should be deleted. However, estimating costs of e.g. libc functions is such a glaring omission, something we definitely should be doing, but we aren't very unfortunately. I had to switch tasks before I could finish D59129, but that is just one example how we could estimate costs.... So yeah, I don't know, if this is really bothering you, we can get rid of rid, but perhaps keeping this infrastructure isn't that bad?
Perhaps this is also a good motivation to pick up D59129 again on the side...

Cheers, LGTM

Cheers, with some nits addressed this looks good.

A few irrelevant nits inlined, but also one question about the test.

Looks reasonable to me.

Thanks for commenting!

2-character changes that fix wrong codegen are always good changes, so have no problem directly LGTM'ing this.

Will do, and many thanks for all your reviews!

• replaced ID.getStep() -> Step
• removed the sentence on line 1899 - 1900, and
• moved the comments on lines 1900 - 1904 to 1889.

Many thanks for reviewing!
I think this has all comments addressed.

Cheers, nice one.

In D76716#1940911, @samparker wrote:

if you change the input and example from 0x00010203 to 0x04010203, then the VMAX will also give a different result after tail-predication

Indeed! Which is why we track for our zero'd false lanes. With vmax being a horizontal operations, we check that it operates upon on registers that we know have zero'd false lanes.

Thanks for that example! I asked this question because I expected the vmax and vmin to behave roughly the same. In your example, if you change the input and example from 0x00010203 to 0x04010203, then the VMAX will also give a different result after tail-predication, or am I still missing something?

Thanks, LGTM

Did a first scan, looks very reasonable, just some first nits/questions inlined.

Yes you're right, the patch that I've made dependent needs this one to work properly.

The goal of this pass is to maximize the size of the VPT blocks created by the MVE VPT Block Insertion pass.

The unit test failure looks genuine, does that needs fixing?

LGTM

Thanks for taking a look @fhahn ! Comments addressed.

Patches with functional changes but without tests are a bit "suspicious". In this case, I see it might be tricky. You could argue that it should be incorporated in the patch that includes the tests, so we can see what's happening. But perhaps separating this out is cleaner, if the other patch is big. But can you at least make the next patch dependent on this one, so we know where this is used?

looks like a good fix to me.

Cheers, looks good to me.

Thanks, LGTM

Looks like a good direction to me.

Looks good to me. Please wait a day in case Eli has more comments.

I am now up to date with this, and don't have any questions about the logic, that looks good.
I would like to go over it once more when (some of) the nits have been addressed as I would like to get an overview how things read and look as this introduces quite a few new things and concepts.

Besides the irrelevant formatting nits, one minor question about the clang test.

Some nits inlined

That sounds good and that seems to address the feedback given here, which I agree with. Just wanted to briefly add that while it already looks like most interested people are on this ticket, perhaps it good to also share the plan and design on the cfe dev list for visibility.

Big patch: this is just a first scan of the code, and a first round of nits. Now going to look again, to let things sink in.

I'm still not sure why __fp16, which is a storage-only type, is used for the element type of float16x4_t if we want to avoid promotion to a float vector type.

With the nit addressed, this LGTM

Cheers, I think this looks very reasonable.

LGTM, with one nit inline.

was just wondering about more corner cases for "negative" tests: do we have test where lanes are swapped (if that makes sense)?

LGTM

Committed in: https://github.com/llvm/llvm-test-suite/commit/2af3f724f9c857587cecbc03f7f88f605d739ea2

I wanted to include these test changes in the recommit, but perhaps it's nice to commit this separately (as this this a change in a different component).
So wanted to check if we are happy with this?

Adding @simon_tatham in case he feels wants to have a look too.

Agreed, they don't swap lanes. They can write to only bottom/top halfs, but that's fine. So looks like a straightforward change to me.

Big patch, I only did a first scan, but looks very reasonable in general. Just a first round of nits and 2 questions.

Thanks for that solution, have copied it to the test file.

Patch for the test-suite: D75557

Thanks James, I will prepare a new revision!
The test-suite needs porting too (see comment in D75056) and am first looking into that, after that I will return to this.

Ha, these test-suite apps fail with multiple definition of ... link errors, so actually require a little bit of porting! :-)
But I think I will prepare a patch that adds -fcommon to their makefiles, as I don't want to change too many things at the same time.

Reverted in: https://reviews.llvm.org/rG4e363563fa14

I probably forgot to add why these tests started failing!

Thanks all for the reviews and help.
I will fix these things and do a last rebase/check before committing this tomorrow.

Nice optimisation, looks very reasonable to me: LGTM

LGTM

Friendly ping, and just checking: are we happy with this? Good to go?

Looks like this needs a rebase?

Thanks, LGTM

Thanks for that! That greatly helps readability!

Are there any tests remaining that check that with -fcommon, IR generation creates "common" variables, now that all these tests have been modified?

That should happen automatically when you put the tag Differential Revision: ... in the commit message.
It doesn't look like a made spell mistake in it, so am a bit puzzled why this didn't happen.... but anyway, it's closed now.

Sure, no problem, this is committed in: https://github.com/llvm/llvm-project/commit/13db7490fa67e22605dec4ab824121230b0fd928

Removed the CHECK-NOTs from some tests as suggested.

Yep, those are the instructions to request commit access, which might be good to request if you plan to do more llvm work. I guess that will probably take a few days. And if you're not in a hurry with getting this committed, you could wait for your commit access and then commit this yourself. But I would of course be happy to commit this on your behalf, which I will then do tomorrow morning, just let me know what you prefer.

minor test update

Thanks for catching that. Now it shows more changes in tests, so updated a bunch of tests.