Addressed Sam's comments:

• set the alignment on the load instructions.
• added alias analysis checks.
• check that the accumulator is the right integer type.
• bail out early when the operand chains of the muls are integers constants or more complicated patterns we don't yet support.

Tests have been modified and added for all these cases.

No problem, will commit this today.

Thanks a lot for the reviews. I will wait a day before committing.

Thanks, more tests added.

Tweaked the unittests a bit more.

I think you can commit this. And should there be more comments, then they can be addressed post commit.

Looks OK to me.

Thanks for the suggestions:

• added unittests,
• now storing both the pointer and value operand (looks most convenient to me).

As discussed with @rengolin and @t.p.northover, simple SVE assembler patches can be directly committed. I think this would be a good first candidate for that :)

Ah, thanks! Will add tests there, and check if I want the pointer operand.

Hi John, thanks for the review! I have made the kernel that drives the transformations explicit regarding the inputs and outputs of the different functions; that indeed reads much nicer now.

I know very little about SPIR, and Initially didn't understand this:

Looks OK to me.

This is addressing:

Hi Sam, thanks for the reviews! This is a tidy-up addressing the nits and minor comments in the 1st review. Not addressed yet is the check for unaligned accesses support, which I will look first into now.

On targets with NEON, we should prefer NEON vectorization (vmlal etc.) over DSP instructions in almost all cases.

Thanks. Looks like a straightforward fix to me.

Nits title:

• Think you can simplify the title to something along the lines of: "[AArch64] Support the FP16 VCVTA_U16 intrinsic". No need to mention tests are added in the subject (tests should always be added).

Showed some decent improvements in a few benchmarks. Cheers.

Looks OK to me.

Thanks, this looks a lot better IMO.

Agreed: supported architectures are v7/A32/A64.
Thanks for fixing this.

Agreed: supported architectures are v7/A32/A64.

The initial commit showed problem in testing, and my fix added the check that the AND has a single use. Looks like indeed that we just forgot to remove the other check as I also don't see why it would be necessary. Thanks for fixing.

I think this looks ok now, just some nits inline.

Hi, I might be missing something, but why do we need this? Why is this not the same as the already existing "k_FPImm" case (with perhaps an additional check somewhere to see if it is one of these 4 expected values)?

Looks okay to me.

Cleanup/simplification of the tests looks good to me. Just a nit inlined.

Like I said in D47121, I agree that these intrinsics are available in v7/A32/A64.
This change looks reasonable to me.

I agree: these intrinsics are available in v7/A32/A64.

Thanks, looks OK to me.

Had only a first brief look; see some first drive by comments inline.

Any suggestions on what would be at the top of the list? :)

Hi Junmo, many thanks for sharing this. Looks like we have some more work and further tuning to do!

This looked very reasonable to me as an initial commit. And I thought so too, like Hal and Eric, that different enhancements can still be made. But as I wrote, we can iterate further on this once we've got something in. So I don't think this was a misstep, and once the buildbot failures are resolved, it looks like this can be recommitted. However, as I also mentioned, another reason for an initial commit is more exposure, potentially more people looking at it. So this discussion is great, the more feedback, the better; especially if there's something that needs to be addressed before a recommit.

Looks OK to me.

Just out of curiousity:

• How do you plan to implement this? Are you going to generate from the pragma some sort of "script" that dictates the transformation order which is going to be fed to the pass manager?
• About the stacking of pragmas, in your example you apply the bottom one first, but would a user perhaps expect the first to be applied? In other words, is the expected behaviour described somewhere (in a spec)?

Yes, excellent, thank you!

Hi Dave,

Looks OK to me.

Looks OK to me.

Looks okay to me.

Thanks, I think this looks OK.

Thanks. This looks OK to me.

Sorry, I had one more look, see question inlined.

Thanks, looks good to me now. One more nit inlined, but no need for another review.

Looks OK to me.

Hi Luke, thanks for fixing this, some first comments inlined.

Sorry, I needed to get up to speed here, also with this hacker's delight trick.
I think it would be good if you add some comments to LowerMULH what we are trying to do here.

Looks OK to me.

Looks good to me.

Looks okay to me.

I think this looks OK.

Looks like a straight forward fix/addition to me.

Looks good to me.

Looks good to me too, just some comments about typos/nits inlined from my side. I will let Oliver approve as he did most of the work in this area.

Looks good to me, just one nit inlined.

Looks reasonable to me.

Hi Dave, thanks for making this into a pass, I think this looks a lot better now. I skimmed through the whole diff for first time, and just wrote down some things I noticed, mostly nitpicks, see the comments inlined. I will now study the different bits and pieces in more detail.

Thanks James!

Thanks, and I am going to try to get some clarity on this doc issue. But looks like it should be "ARMv7, ARMv8", as it used to be. Make sense to comment on this in the commit message, if that's what you mean.

Sorry, I have second thoughts on this.
This seems more like a doc issue than anything else. There is no reason why this could not be supported in A32. GCC is also supporting this, and removing it is a bit user unfriendly.
Would you mind reverting this?

The corresponding LLVM tests seem be there already, so this looks all good to me.

Yep, agreed, also on the new shiny https://developer.arm.com/technologies/neon/intrinsics it is listed as A64 only.

This looks correct to me, the ACLE indeed says that:

Not really familiar with these 2 intrinsics, I had a quick look at the ACLE:

Thanks, I've updated the tests with more checks.

LGTM, thanks.

I forgot to mention, that due to your last comment, I had a closer look at the generated code and spotted a missed optimisation. Therefore I've also added a new pattern, as it was not always generating a vmin/vmax pattern. I've changed the "safe" tests to check the actual expected pattern; before the tests were not really great because they were not testing the full pattern.