• Fixed legalization action code, which was trying to use widening for all integer scalable vector types after the new MVT ranges were added
• Added unit tests for legalizing the result via promotion
• Minor cleanups, additional comments as requested

Ping.

Hmm, forgot to add the last round of minor fixes before committing. Sorry about that, will push them as well.

• Added DAG combine to canonicalize a build_vector splat into a splat_vector if the target supports the splat_vector operation (by overriding the default Expand legalization action)

Hi James,

• Renamed ScalableSize to TypeSize, including header name.
• added alignTo function that takes and returns a TypeSize. I wasn't sure if this should be added to MathExtras.h where the other variants live, so just kept it in TypeSize.h for now

• Removed intrinsic and supporting code
• Added new ISD as discussed on llvm-dev: http://lists.llvm.org/pipermail/llvm-dev/2019-August/134809.html

You can add the extra multiclass for ftsmul if you wish, but it's not needed until someone implements a matching pattern for that instruction. We only match it against an ACLE intrinsic downstream, not common SDag nodes.

Thanks for taking an interest in SVE codegen. The tests look fine, but we had a different approach in mind for generating many of the patterns. In this case instead of making separate def : Pat< lines for each type, we add an SDPatternOperator and pattern to the multiclass for that instruction type.

• Changed existing interface to return ScalableSize objects and added a (mostly) transparent conversion, as per Sander's suggestion.
• Removed new interfaces for DataLayout and Type.
• Fixed cases where the transparent conversion doesn't quite work (e.g. std::max/min, where the types must be the same)

In D66871#1649148, @cameron.mcinally wrote:

LGTM

Does anyone like Sander's suggestion to make ScalableSize (or whatever we end up naming it) the return value for all size queries and provide an overloaded cast operator to transparently work with existing code comparing against unsigned values? Or is it preferable to keep the current split?

• Split out backend code into separate patches
• Renamed 'Min' to 'KnownMin' in method names.
• Added a few more comments.

• Reordered MVTs to group fixed-length types together (and the same for scalable).
• Replaced existing range accessors to remove explicit isScalableVector checks on MVTs in various backends.

Split out from D53137.

Created D66339 to split out the code to skip scalable vector types in other backends.

In D53137#1621351, @sdesmalen wrote:

Thanks @huntergr for working on this!

This patch can probably be split into two separate patches, which make them easier to review;

• One that fixes the Targets to ignore scalable types (see comment)
• Another one that adds the interface for scalable size queries.

• Added comments explaining the new methods
• Added tests for the MVT/EVT/DataLayout interfaces
• Refactored ScalableSize class operators to build on '==' and '<'
• Added checks in various backends that don't support scalable vectors to prevent legalizing operations involving scalable MVTs

• Added inline comments to clarify boolean parameter meaning.

Thanks for the reviews; I left a couple of inline comments, and will make the requested changes.

• Updated to head
• Removed MVT-specific ElementCount in favour of using the common struct
• Added unit tests.

• Removed most of the changes in favour of reintroducing them in separate patches later with appropriate tests.
• Added tests for core IR instructions to make sure they don't drop the scalable flag.
• Fixed up a couple places which broke the new tests.

Hi Diana,

Updated based on where scalable size queries were required when running with an SVE-capable loop vectorizer on several codebases (all of LNT, various flavours of SpecCPU, lots of HPC and embedded benchmarks).

In D32530#1535948, @rengolin wrote:

@huntergr do you have an account on bugzilla? I couldn't CC you on that bug.

In D32530#1535665, @thakis wrote:

Hi, this slowed down thinlto links 3-4x, and other things probably too. PR42210 has details. I've reverted this for now in r362913.

• Simplified wording for extract/insertelement semantics

• Changed textual IR format to <vscale x n x <ty>>

It seems like there's enough support for changing to <vscale x as the prefix, so I'll revise the patch.

• Changed (extract|insert)element semantics to return poison values at runtime if the index exceeds hardware vector length.
• Changed shufflevector semantics to note that only zeroinitializer and undef can be used as mask values for scalable vectors for now.

In D32530#1488001, @cameron.mcinally wrote:

If you know the exact size of your vectors at compile time, then I believe fixed length vector types should be used, at least at the IR level.

Ah, ok. So let's say we're targeting SVE and I know my vector length is 512b. I was imagining building a vector like this:

%r1 = insertelement <scalable 2 x double> undef, double %x, i32 0  
%r2 = insertelement <scalable 2 x double > %r1, double %x1, i32 1
%r3 = insertelement <scalable 2 x double > %r2, double %x2, i32 2
%r4 = insertelement <scalable 2 x double > %r3, double %x3, i32 3
%r5 = insertelement <scalable 2 x double > %r4, double %x4, i32 4
%r6 = insertelement <scalable 2 x double > %r5, double %x5, i32 5
%r7 = insertelement <scalable 2 x double > %r6, double %x6, i32 6
%r8 = insertelement <scalable 2 x double > %r7, double %x7, i32 7

In D32530#1487868, @cameron.mcinally wrote:

Sorry to be late to the party, but I have a quick question:

Ok, so do we have agreement that constant literal indices should be limited to 0..n-1 for now, but non-constant indices can potentially exceed n so that expressions featuring vscale can be used?

What if we know the width is fixed on our target machine? Let's say it's fixed at 512b. So a full width scalable double vector would be:

<vscale x 2 x double>

Since our width is fixed, we know that vscale=4 here and there are 8 elements in this vector.

I'd like to be able to do a really fast insert at say index 3. I.e. insert_element(<vscale x 2 x double> res, double elt, int 3). Would that insert be as fast with the vscale method as it would be for an index < n-1?

In D32530#1486007, @hfinkel wrote:

In D32530#1485862, @rkruppe wrote:

In D32530#1485818, @huntergr wrote:

In D32530#1484668, @hfinkel wrote:

Why implementation defined and not UB for the case where the index exceeds the runtime length? How do you intend to define this for SVE?

SVE uses a predicate for indexed inserts and extracts. We generate that predicate by comparing a splat of the index against a stepvector (0,1,2,3...); if the index is out of range then the predicate will be all false.

For a mov (insert), that results in an unmodified vector.

For a lastb (extract), that extracts the last lane in the vector if no predicate bits are true.

I don't know if RVV or SX-Aurora have similarly defined semantics. If the preference is to make it UB, that's fine.

I think this should be UB, or at least return poison. However, making it UB has the unfortunate side effect of making it illegal to hoist these operations out of conditionals (which currently isn't the case), so maybe poison is better.

For me the main reason for making is UB (aside from generally being conservative) is that element insert/extract can be usefully legalized by putting the vector into a stack slot and accessing the affected element with scalar loads/stores -- in fact, that's the default legalization strategy in trunk. But in that setting an out-of-bounds lane index would become a wild read/write (= clear-cut UB), unless the legalization code jumps through extra hoops to add a bounds check or clamp the lane index into range. This wouldn't affect SVE or RVV since they would implement custom lowering anyway, but for other targets (especially if we eventually generalize insert/extract to accept variable lane positions and do so for all vector types for consistency) it could become an unnecessary headache. Though if we make insertelement with out-of-range lane index return poison instead of being UB, then at minimum clamping in insertelement is necessary to avoid the wild write.

Aside: your proposed semantics for SVE would break the property extractelt(insertelt(vec, i, elt), i) = elt which instcombine (and others) most likely assumes. If we make the insertelt return poison (or even just undef, FWIW), that issue goes away.

I think that making the return be a poison value is best.

In D32530#1484668, @hfinkel wrote:

In D32530#1484027, @huntergr wrote:

Noted the change in semantics for extractelement and insertelement in the langref.

Why implementation defined and not UB for the case where the index exceeds the runtime length? How do you intend to define this for SVE?

Noted the change in semantics for extractelement and insertelement in the langref.

In D32530#1477261, @hsaito wrote:

In D32530#1477164, @rengolin wrote:

In D32530#1477041, @hfinkel wrote:

Exactly. Non-constant values can become constant. Constant values can be guarded by vscale-dependent runtime guards (both hand-written and compiler generated). My preference is to leave this not restricted to vscale == 1 values, but rather allow all values that can be supported at runtime, and have it be UB if, at runtime, the relevant index is not available.

+1

That means a need for a warning to general developers: If there is a check for constant_index >= n to see if the result is a poison value, that code has to be changed so that it applies to non-scalable vector only. Hopefully not too many instances of that. I'm fine with this as long as the communication to the rest of LLVM community is clear about it.

In D32530#1474555, @greened wrote:

In D32530#1470784, @hfinkel wrote:

In D32530#1470773, @greened wrote:

I am not sure how it could be anything but n. If you don't know how long the vector is, you can't correctly generate an index beyond n.

But you know at runtime... there has to be a way to determine, at runtime, vscale. And the index doesn't need to be a constant. I'm not sure that we need to restrict non-constant n to only values valid for vscale == 1.

Good point. 100% agree. I was only considering the constant case.

In D32530#1470784, @hfinkel wrote:

In D32530#1470773, @greened wrote:

We need to clarify on insertelement/extractelement. Maybe already done in some other patches, but that clarification should be part of this patch.
Is the "length of val" under the semantics "scalable * n" in <scalable n x ElemTy>, right? Or is it still n?

I am not sure how it could be anything but n. If you don't know how long the vector is, you can't correctly generate an index beyond n.

But you know at runtime... there has to be a way to determine, at runtime, vscale. And the index doesn't need to be a constant. I'm not sure that we need to restrict non-constant n to only values valid for vscale == 1.

In D32530#1470773, @greened wrote:

We need to clarify on insertelement/extractelement. Maybe already done in some other patches, but that clarification should be part of this patch.
Is the "length of val" under the semantics "scalable * n" in <scalable n x ElemTy>, right? Or is it still n?

I am not sure how it could be anything but n. If you don't know how long the vector is, you can't correctly generate an index beyond n. I assume for vectors of length > n one would have to use shufflevector or something similar (using vscale and stepvector as Graham mentioned) to get the elements you want to the lower n positions.

Either way, the semantics and any restrictions certainly need to be documented.

In D32530#1468145, @simoll wrote:

That's pretty much what LLVM-VP is about: https://reviews.llvm.org/D57504

We are proposing compress/expand and lane shift as intrinsics.
I suggest you add any shuffle intrinsics to the same namespace to avoid fragmentation.

In D32530#1464722, @hsaito wrote:

In D32530#1463735, @joelkevinjones wrote:

I think this is a coherent set of changes. Given the current top of trunk, this expands support from just assembly/disassembly of machine instructions to include LLVM IR, right? Such being the case, I think this patch should go in. I have some ideas on how to structure passes so SV IR supporting optimizations can be added incrementally. If anyone thinks such a proposal would help, let me know.

I think there is one more thing we still have to do. Does scalable vector type apply to all Instructions where non-scalable vector is allowed? If the answer is no, we need to identify which ones are not allowed to take scalable vector type operand/result. Some of the Instructions are not plain element-wise operation. Do we have agreed upon semantics for all those that are allowed?

• Clarified that the runtime multiple is constant across all scalable vector types, even if the constant value isn't known at compile time.
• Removed extra whitespace.

I think this patch is a good start and I don't mind having it merged as is, I was just suggesting a way forward since it seems people are still hesitating. While there seems to be some consensus on moving forward with native types, I'm sure many of the details are still somewhat fuzzy. In my opinion having more patches reviewed would make those details clear, and it would thus make it easier to get more sign-offs on this patch as well.

I'd advise caution here, it's really significant/impactful change, and a single sign-off is a bit worrying.
In particular, even regardless of the feature itself, has the implementation itself been reviewed?

In D32530#1448135, @sebpop wrote:

I am accepting the scalable vector types based on the comments in
http://lists.llvm.org/pipermail/llvm-dev/2019-March/131137.html

Let's move forward with SVE support in LLVM. Thanks!

In D31417#1450587, @xtian wrote:

LGTM. This provides a consistent behavior same as GCC and ICC w/ -fopenmp-simd option. To answer, Kelvin's question. it is not directly tied with "target".

A more focused set of unit tests will be created instead of this demonstration test.

A better set of patterns will be created later.

A better set of patterns will be created later.

A better set of patterns will be created later.

Needs to be updated with better isel patterns (and possibly a new ISD?), instead of just the bare minimum required for demonstration.

Needs to be updated with better isel patterns, instead of just the bare minimum required for demonstration.