In D94708#2501255, @cameron.mcinally wrote:

In D94444, @paulwalker-arm proposed a more generic extract vector intrinsic that accepts an index and stride. Now I'm wondering if we should just have a generic scalable shuffle vector intrinsic to handle all these operations under one intrinsic.

That idea doesn't need to hold up this Diff, but it might be something to consider...

A bit of a flyby review as I'm still on holidays but to my mind many of the restrictions being proposed for the new intrinsic seem purely down to the design decision of splitting the input vector across two operands. I understand this is how the underlying instructions work for SVE but that does not seem like a good enough reason to compromise the IR.

Thanks @david-arm . FYI the CHECK-DAGs within sve-fixed-length-int-arith.ll don't need to be DAGs but I'm going to restructure these tests anyway so there's not point changing them.

Please can you add entries for nxv2f16 as well? That way all the legal fp types are covered.

I'm just keeping things ticking over. It doesn't have to be this patch, I just did wanted to know if it's something I can take off my TODO list. If you're support eager then FABS also requires fixed length support :)

Do you plan to add support for fixed length vectors?

As before I still believe there should be a test to protect this fix as presumably you're doing it for a reason.

Fixed incorrect variable naming within ctlz_v64i8, ctpop_v64i8, cttz_v64i8.

Fixed incorrect variable naming with bitreverse_v64i8 test.
Made test function names consistent.

Is that not a bug in WidenVecRes_MGATHER? I would have thought there's an expectation that the element count of MGATHER would match the element count of it's MemVT. That's to say that these nodes allow loads with element extension rather than loads where the vector is packed into a larger container.

I'll wait to give other reviewers a chance to comment, but otherwise I'll commit it for you tomorrow.

Other than some missing tests this looks reasonable to me.

Just making a note here that I've push D90093 so that we follow the same principle shown here for the lowering of i1 based int_to_fp conversions. This also means getPromotedVTForPredicate is now useable for this patch also.

A few stylistic things to consider (I'm only really bothered about loosing the v32 tests) but otherwise looks good.

In D92760#2442179, @david-arm wrote:

If you can find a test that exposes the need for this split function with generic IR that would be really helpful I think - LLVM has managed for a long time without needing this split function so something has changed. I suspect it's probably just the new intrinsic that now exposes this code path.

I agree with @david-arm because this unsigned->class conversion has a different rational than TypeSize. Here we want to have the ability to allow natural maths when computing a cost but also be able to reflect the state where the cost is special, one example of which is "the cost is meaningless", rather than trying to second guess the callers intention and say "just returning a big number in the hope the user does what we need".

Change looks sensible but please add a test.

As I see it there are a bunch of pragmas that all enable vectorisation, with each pragma providing a unit of information. One component of this information is the vectorisation factor hint provided by vectorize_width.

Thanks for the info @david-arm. I just figured we'd support vector_width(2), vector_width(2, fixed), vector_width(2, scalable), vector_width(fixed), vector_width(scalable) so I still say splitting the width property across multiple pragmas is against our goal of moving away from fixed length only representations. That said, if this is the consensus then so be it.

Hi @david-arm, can you document the reasons for the change?

Thanks @lebedev.ri, I incorrectly assumed exposing existing functionality wouldn't require an RFC. I'm putting together an RFC to cover support for other scalable vector shuffles so I'll include these within that.

@lebedev.ri: shufflevector only has minimal support for scalable vectors with only the splat case covered (and even that has its quirks). With the recent change to force the mask to be an ArrayRef there is no way to represent arbitrary shuffles and at the same time the implementation forced a requirement that scalable vector data inputs imply a scalable vector result.

Presumably you're planning to add some tests?

Just a couple of general observations as I'm not sure if I'll get chance to properly review before the patch is accepted. I'm not expecting any changes based on these comments as they're effectively refactoring and thus getting to a state where MGATHER/MSCATTER works for SVE is a better starting point for such refactoring.

I've created this patch so as to be consistent with D87651.

With this patch[1] landed I believe operation legalisation is now a solved problem for SVE (well for fixed length vectors). I think it's worth tackling the type legalisation side of things rather than overcomplicating shouldExpandReduction. Of course that's easy for me to say given it's your time :) but you've already done part of the work based on the older patch.

@cameron.mcinally: I'm sure you know this but just in case it saves some time I can confirm you will not hit the splitting code until after you relax shouldExpandReduction. So I think your current patch is complete so it really comes down to whether adding the support this way round (i.e. only allow legal types for SVE, then allow all types and implement the legalisation) is acceptable to @nikic .

In D89162#2350313, @nikic wrote:

@paulwalker-arm There is no need to implement any target-specific support. The legalization outcome will be a simple chain of extracts and fadd/fmuls. It does not need to generate good code, just not assert for any VTs.

@nikic Why does wanting to implement VECREDUCE_SEQ_FADD for SVE necessitate having to implement full support for NEON (AArch64 and Arm)?

In D89162#2347814, @cameron.mcinally wrote:

The new tests would be broken without the legalisation changes, so I'm assuming that those are enough coverage. Maybe I'm missing something though...

I just don't buy this argument. You're saying that we must force all target's to perform custom lowering for promotable illegal operations on i1 vectors because somebody in the future might try to "fix" LegalizeDAG when they should implement custom lowering for extend/truncate operations. This is the situation Target/AArch64 is in and we never considered "fixing" LegalizeDAG so I don't see why others wouldn't just follow our example.

Sorry @cameron.mcinally I've not had much time for code reviews this week although will take proper look tomorrow. I have a question though. You've added extra legalisation support but I don't see any explicit tests (or at least ones with matching check lines) for it. Is this something you need for this patch? (I'm guessing sve-fixed-length-fp-reduce.ll's stock NEON run line triggers the cases?) If so then there really should be a neon specific test file that verifies the widening and scalarisation changes as the NEON run line for the "fixed-length" tests is more about ensuring no SVE instructions slip through.

Perhaps I'm missing something but I've created D89950 to show what I mentioned in my previous comment. To me it's preferable to have this as target independent code as it seems a common enough solution. I know it results in "Pomote" having multiple meanings but that boat has sailed because different nodes have already chosen different meanings and I think it's pretty clear from the old and new VTs what's being asked for.

I guess most of my comments relate to minimising direct uses of TypeSize.

Not lowering to SVE for v2f## MVTs makes sense for now but as before when we have proper support for v#i1 our hands will be tied.

I think we should keep this because it gives us the possibility to attach it to functions where the function signature wouldn't use it otherwise.

Looking at the testing I think there are some holes regarding general vector inserts, but I'll investigate that under a different patch.

Are you planning to add support for the normal VECREDUCE_FADD? I ask because that's likely to see more initial use upstream than the SEQ variant.