- User Since
- Nov 24 2016, 5:21 AM (204 w, 3 d)
I've created this patch so as to be consistent with D87651.
Fri, Oct 23
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.
Thu, Oct 22
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.
Tue, Oct 20
Fri, Oct 16
Thu, Oct 15
I guess most of my comments relate to minimising direct uses of TypeSize.
Wed, Oct 14
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.
Tue, Oct 13
Mon, Oct 12
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.
Thu, Oct 8
Tue, Oct 6
Mon, Oct 5
Fri, Oct 2
Re: OverrideNEON: I wouldn't get too hung up on this. The original intention was me trying to reduce the chances of going down broken code paths and also not surprise people with a complete change of code generation output (a.k.a Operation "get something that's usable quickly"). Once wide vector support is driven by function attributes and we start adding proper v#i1 support, we'll be forced to breakaway from NEON and OverrideNEON will become a distant memory.
Thu, Oct 1
Pretty much by the number but note that I've omitted ISD::FROUNDEVEN because support for that is already lacking for NEON.
Wed, Sep 30
Don't suggest promotion for 'vscale x 16' lane floating point operations.
Tue, Sep 29
@cameron.mcinally No worries, I clearly didn't do the best job reviewing that patch either.
FP_ROUND's extra operand is a tad inconvenient but that's not your fault and you're following the naming rules for _MERGE_PASSTHRU so this LGTM. I'll note there's no illegal to illegal test for fptrunc like there is for fpext but then I don't know what those tests are verifying that's not already covered by the illegal<->legal tests.
Mon, Sep 28
Just a heads up that @kmclaughlin is starting to look at legalisation/lowering for scalable vector types. From an operation legalisation point of view you've done most of the plumbing so hopefully any toe treading will be minimal.
Sep 25 2020
Formatting issues aside this LGTM.
LGTM assuming the potential compiler warning is removed.
@david-arm The main priority is to remove the operator overloads so if universally using divideCoefficientBy pleases the most people then let's just do that.
Other than a quibble related to singleton DAG checks this patch LGTM.
Sep 24 2020
I think the precedent has already been set as VectorType and ValueType already deem doubling and halving to be special enough to have explicit operations, albeit they save more effort that what's being saved here. Personally I just think
is more meaningful/readable than
Whereas ElementCount * 2 is already meaningful enough.
Sep 23 2020
I'm not a great fan of the coefficientDiv name but once in I doubt I'll give it a second thought. That said, because by far the most common usage is coefficientDiv(2), whose intent is clearly to knowingly split something into two equal parts I'm wondering if adding a halve() utility function will keep the main usage short and meaningful and then there's even less reason to care about the name of coefficientDiv.
My preference would be to follow the clang-format advice for TypeSize.h.
Sep 22 2020
Sep 21 2020
Sep 18 2020
Replaced custom selection with isel patterns. Since we're going the isel route I figures I may was well add the missing patterns for unpacked floating point types.
Sep 17 2020
Remove block that is incorrectly reporting unpacked and predicate EXTRACT_VECTOR_ELT as legal.
Oh, I hadn't realised we are handling reduction in this way upstream (although it does match an old design we had downstream). This is certainly not the expected behaviour so I'll get it fixed. The expectation is for the SVE reduction ISD nodes to reflect the underlying instructions behaviour, which is they set the whole vector register. The reason for this is that we don't want the element extraction to be done during isel because it introduces needless vpr-gpr transitions and there are also use cases that make use of the implicit zeroing of the upper lanes.
Sep 16 2020
Oh and the commit message should reference the lowering of FP_TO_SINT/FP_TO_UINT.
LGTM assuming adding the missing tests don't throw up anything unexpected.
Sep 15 2020
Sep 10 2020
Sep 9 2020
Looks good assuming the new test doesn't throw up any surprises.
Sep 8 2020
I'm afraid some of my comments are probably conflicting. I would have experimented a little to understand better how things work, but I'm guessing the patch is based on other work because I couldn't some of the affected functions/nodes.
Sep 7 2020
Sorry I had hoped to look at this patch properly today but that hasn't worked out. I've got a general comment below as I think it would be nicer to abstract the NativeSaturation into a TLI hook that other targets can implement when useful to them.
Sep 3 2020
Other than adding the missing test before landing that patch, this LGTM.
Sep 2 2020
With the exception of VSELECT lowering, which is being worked under D85364, everything else is available in master.