Don't suggest promotion for 'vscale x 16' lane floating point operations.

@cameron.mcinally No worries, I clearly didn't do the best job reviewing that patch either.

In D88444#2299066, @cameron.mcinally wrote:

Also, while I have everyone's attention, there are a number of unhandled vector reduction intrinsics with SVE support. Do we want to add lowerings for those? E.g. ANDV.

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.

In D88317#2298070, @cameron.mcinally wrote:

Or, if @kmclaughlin would like, I can add the Scalable lowerings while I'm here. Assuming you don't have downstream changes prepared already...

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.

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.

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

ElementCount.halve()

is more meaningful/readable than

assert(ElementCount.isKnownMultiple(2));
ElementCount.coefficientDiv(2);

Whereas ElementCount * 2 is already meaningful enough.

In D88033#2288287, @efriedma wrote:

LGTM

Alternatively, we could make CONCAT_VECTOR "legal", and lower it using an isel pattern. But I'm not sure that's actually an improvement.

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.

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.

Remove block that is incorrectly reporting unpacked and predicate EXTRACT_VECTOR_ELT as legal.

D87843 fixes the issue. Its dependence on D87842 is purely to maintain current code quality.

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.

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.

In D87707#2274650, @Asif wrote:

@paulwalker-arm: I couldn't find out the corresponding ISD Node for FRECPX.

Looks good assuming the new test doesn't throw up any surprises.

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.

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.

Other than adding the missing test before landing that patch, this LGTM.

With the exception of VSELECT lowering, which is being worked under D85364, everything else is available in master.

The intention of this patch is now complete. All work is available in master with the exception of the hook into -msve-vector-bits which is not necessarily the direction we'll use once function attributes are available.

Fair enough, thanks for the information Owen.

Sure, but that means there need to be a way to detect when FP is displaced (hence my original question). If such data is available (and it needs to be for SVE code generation to function correctly) then I don't see why the assert cannot use it to assert Bytes is correctly aligned and displaced.

But doesn't that mean the assert is now protecting less than it was? Without the knowledge of FP's displacement, you cannot know if Bytes % 8 == 0 is safe or not. Is the knowledge of FP's displacement recorded anyway so the original intent of the assert is not compromised? From an SVE point of view, FP displacement will either need to be prevented or undone if it's used to access SVE stack slots. This is because SVE offsets are implicitly scaled and thus any byte based displacement cannot be encoded into its instructions.

I've had to revert this patch because it caused runtime failures when building spec2k/eon with -march=armv8-a+sve -mllvm -aarch64-sve-vector-bits-min=256.

@ro Is it worth specifying an end date for votes/opinions?

My vote hasn't changed, I mean I already accepted the tweaked Option1 so you could have been done and dusted by now :) I just don't see what's gained from Option3's refactoring. It does the same as Option1 but in a less type safe/flexible/c++ way. Furthermore, the new interface is needlessly different to the other functions that handle SVE immediate values when the vector element type plays a role.

To be more clear, I'm happy to defer the divide conversation for if/when we run into issues so my previous acceptance still stands. It'll be good to get the intent of the patch in (i.e. stoping access to internal class members) asap, plus any follow up work will be a smaller more manageable patch. It's worth talking this through during the next sync call to see it we can get some consensus regarding what maths is and isn't allowed.

I'm retracting my operator% request. After thinking about it and speaking with Dave I just cannot see how allowing a total divide is safe for scalable vectors. If you are relying on a truncating divide then special handling is require anyway, which is likely to be different between fixed-length and scalable vectors.

Can't say I agree since people are already writing the ugly code, because the result typically demands different handling or they're asserting the divide doesn't truncate in the first place. That said I'm happy for there to be no assert as long as operator% is implemented so users can calculate the remainder in the expected way.

There's probably a few .Min to .getKnownMinValue() conversions where the .Min could be dropped (calls to Builder.CreateVectorSplat for example) but they can be tidied up as part of a proper activity to reduce the places where getKnownMinValue is called. So other than my suggested updated to EC::operator/ the patch looks good to my eye. Please give other reviewers a little more time to provide other insights.

I cannot say whether such questions make sense without a deeper investigation, but I can say for certain that EC.isPowerOf2 is a question we cannot answer at compile time. Given this is a mechanical change I would just remove the member function and leave the code as is (well change EC.Min to EC.getKnownMinValue()). We already know that we'll need to visit the places where getKnownMinValue() is used to ensure the question makes sense in the face of scalable vectors.

In D85364#2234543, @cameron.mcinally wrote:

However, when we go to lower this to AArch64ISD::SIGN_EXTEND_INREG_MERGE_PASSTHRU, that operation expects a half vector type for the operand. Here's a special case -- for illustrative purposes only:

Personally I think it's better to order the FCEIL_MERGE_PASSTHRU enum entry and related code alphabetically relative to FNEG_MERGE_PASSTHRU but it's not a deal breaker. So if you're happy with renaming AArch64fceil_mt before landing the patch then I'm happy.

Given you are making use of the fact that AArch64 fp_to_int already does the saturation part, I'm wondering if the different-width saturation can be moved from the input to the output? Specially I'm thinking this will result in nicer code as the min/max immediate values will be easier to produce in integer form.

@Asif & @dancgr : Sorry if you've already started looking at FNEG. It wasn't really my intension to implement support but it turned out to be the nicest way to test the isConstOrConstSplatFP change, which is required to fix an infinite legalisation hang when using fixed length vectors for SVE.

Perhaps I've misunderstood but based on Cameron's original message I suspect he's hit a bug because I lowered all SIGN_EXTEND_INREG operations regardless of the inreg type. This is wrong because there's no patterns for non-power-of-2 non-byte-based inreg types and thus I guess Cameron has hit a selection failure?

The expanding is because we don't yet attempt to lower the subvector and concat_vector operations. As I say, I think today the correct move is to take this patch and then see what the future holds when we have full support for subvec and concat.

In D86114#2227054, @cameron.mcinally wrote:

In D86114#2226955, @efriedma wrote:

The end result is a big uglier than I would have hoped for, but I can't think of any particularly better way given the constraints.

Agreed. I'll wait for Paul to have a look to see if he has any tricks.