LGTM

LGTM

Code looks good to me, but a small nitpick on the function name

It's been longer than I would've liked before I've gotten back to this, but I've;

I wasn't aware of the implicit truncation. In that case, LGTM

Looks good overall, with potential wins in both performance and codesize. Just 1 minor thing (and a nitpick about a comment).

Thanks all for the comments so far (And thanks Dave for taking on the evening shift, as it were)

In D114174#3307302, @dnsampaio wrote:

...
I'm not exactly sure why to do target specific pattern matching here. We could simply add generic complex intrinsics and the different patterns could be matched at each archs ISEL, no? I do agree that it should check in the backend if it should generate a complex operation of a given vector type.
...
Is there any strong reason to create target specific intrinsics instead of having generic intrinsics with polymorphism? Such as llvm.complex.add.v2f32?

The supportsComplexArithmetic method doesn't add a lot on its own. It would probably be better to represent it in terms of asking the backend if it has certain patterns of certain types available. An f32 fcadd, for example, or a i16 fcmul_with_rotate (I'm not sure what to name them).
The ComplexArithmeticGraph seems like internal details to the pass. It would be best not to pass it over the TTI interface boundary if we can. The backend just needs to create the correct intrinsics of the correct type.
Equally I would keep all the matching logic in the pass, not through matchComplexArithmeticIR. That would be similar to, for example, the MachineCombiner pass which has its own enum of a set of patterns.

Unfortunately, not all architectures/intrinsics follow the same patterns, so this was done to provide a way for architecture-specific stuff to be handled as needed. The MVE halving complex add (which I only now realise was included in this patch) is a good example of this; It only works on integers, and requires some extra instructions to be matched. This instruction is only in MVE, it is not part of NEON in any way.
The intrinsics that are common are also constructed differently; While the NEON intrinsics are separated out by their rotations (aarch64_neon_vcmla_rot0, aarch64_neon_vcmla_rot90), MVE squishes them into one and determines the rotation based on a parameter.
ComplexArithmeticGraph is used to encapsulate all the data the TTI might need in order to correctly generate the right intrinsic. It could be replaced by something a bit lighter (and that doesn't expose the implementation detail of the node list), but something will be needed to provide this info.

Applied feedback received offline, redesigned the interface/implementation, and fixed up a number of fundamental issues in the previous iteration.

Fixes the test to pass with this patch in isolation (i.e. without the child revision on top)

In D114590#3166112, @dmgreen wrote:

It may be better to fold this into D114879 so that there is a single patch that can be tested properly.

In D114590#3163780, @SjoerdMeijer wrote:

Okay, that's fine. Perhaps upload your WIP patches too then, stack them, so that we can actually see the (full) approach?

I've got a follow-up patch nearly ready with tests, yep. I want to settle on an approach here before implementing the other side of it.

In D114590#3153774, @dmgreen wrote:

Do we really want to add this to the base Align class? Or should it just be part of whatever controls the code alignment?

In D114174#3142048, @SjoerdMeijer wrote:

Nice one Nick.
Can you first run clang-format on this patch? It is pretty much unreadable with all these lint messages.

LGTM

Updated test

In D109323#2985421, @dmgreen wrote:

OK. Can you add a test? Something that would schedule instructions apart before and keep them together now.

Addressed comments, adding additional test cases that cover cases with out-of-range indices.

LGTM

Apart from a personal gripe (which I feel shouldn't stop this being merged), LGTM

LGTM

LGTM

No issues that I can see, LGTM

In D97947#2700960, @dmgreen wrote:

I was under the impression that without a -mcpu it defaulted to cortex-a53 schedule. It looks like it's no-schedule though, which still counts as an in-order core as it has no MicroOpBufferSize. Can we check if ST->getSchedModel().ProcID != 0, which will be the "NoSchedModel".

In D97947#2698661, @fhahn wrote:

So does this mean the new behavior will be the default if no CPU is specified? I'm not sure if we are ready for that yet, unless we are confident that the current heuristics work well for out-of-order cores too. (Last time I benchmarked this for out-of-order cores there were a few notable regressions, but it's been a few months since then)

Can you remind me about the impact of this? I.e., if -mcpu is omitted, we default to generic which is classified, or is using, an in-order schedmodel description on Android?

The very quick benchmarks I ran didn't show this to be great because of all that extra unrolling, mostly of remainder loops by the look of it. I'm sure you have been running more benchmarks, and perhaps the ones here are not very representative of general A64 code.

In D97947#2612301, @dmgreen wrote:

This will still unroll the remainders of vectorized loops, which will be quite common but generally unhelpful to unroll. Can we try to prevent unrolling there too?

It seems to be difficult to identify the remainder loop; Checking the llvm.loop.isvectorized attribute (like in ARMTargetTransformInfo) caused all gains to be negated, and caused a slight regression when compared to without this change. I've instead included a check for the llvm.loop.unroll.disable attribute, which was seen on the remainder loop IR. This check causes no difference to the benchmark numbers. (Though thinking about it now, that might be due to it being handled elsewhere, making this check redundant)

I've done some more benchmarks with the provided suggestions;

If this version is working that's great, but check it's still OK.

just have a brief look if there are any meaningful codegen difference in e.g. TSVC/Searching-dbl/Searching-dbl.test.

I don't think I would mind merging this with D95632 to keep things in one place; don't think separating brings much value in this case.

Abandoning this and combining it with D95631.

Addressed comments

In D91271#2497380, @mstorsjo wrote:

This broke compilation of some sources...

Addressed comments; modifying checks and updating tests.

In D94234#2484467, @fhahn wrote:

this needs a test?