- User Since
- Nov 24 2016, 5:21 AM (216 w, 4 d)
Sat, Jan 16
Fri, Jan 15
Thu, Jan 14
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.
Wed, Jan 13
Thu, Jan 7
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.
Wed, Jan 6
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?
Tue, Jan 5
As before I still believe there should be a test to protect this fix as presumably you're doing it for a reason.
Sun, Dec 27
Tue, Dec 22
Mon, Dec 21
Fixed incorrect variable naming within ctlz_v64i8, ctpop_v64i8, cttz_v64i8.
Fixed incorrect variable naming with bitreverse_v64i8 test.
Made test function names consistent.
Sun, Dec 20
Dec 18 2020
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.
Dec 17 2020
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.
Dec 16 2020
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.
Dec 15 2020
Dec 11 2020
Dec 10 2020
A few stylistic things to consider (I'm only really bothered about loosing the v32 tests) but otherwise looks good.
Dec 9 2020
Dec 3 2020
Nov 30 2020
Nov 26 2020
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".
Nov 20 2020
Change looks sensible but please add a test.
Nov 18 2020
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?
Nov 17 2020
Nov 13 2020
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.
Nov 12 2020
@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?
Nov 4 2020
Oct 29 2020
Oct 28 2020
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.
Oct 24 2020
I've created this patch so as to be consistent with D87651.
Oct 23 2020
With this patch 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 .
@nikic Why does wanting to implement VECREDUCE_SEQ_FADD for SVE necessitate having to implement full support for NEON (AArch64 and Arm)?
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.
Oct 22 2020
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.
Oct 20 2020
Oct 16 2020
Oct 15 2020
I guess most of my comments relate to minimising direct uses of TypeSize.
Oct 14 2020
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.
Oct 13 2020
Oct 12 2020
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.