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.

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

Rebased, incorporated fixes from reviews.

Update based on off-list feedback:

• Added a section on allowed operations to clarify this is a first class type.
• Remove initial proposal on the flag for not inheriting vlen; there's more pushback against allowing runtime multiple changes. I think this design could still be extended to support that in the future, but I'm afraid I'll have to leave that battle to the RVV team.
• Minor wording changes.

• Unified ScalableSize representation
• Changed to uint64_t + boolean, since we no longer allow scalable vectors in aggregates
• Removed aggregate and mixed unit tests

• Added checks in verifier to prevent scalable vectors being included in structs or arrays
• Changed lookup to use ElementCount
• Moved ElementCount to a new file
• More unit tests

Updated based on feedback:

• Add reasons for restrictions on global/aggregates
• Change size query struct to an integer + a boolean instead of two integers.

In D53695#1277187, @rengolin wrote:

Also, can you move this to docs/Proposals, together with the other RFCs, please?

Updated based on discussions at the 2018 devmeeting

Abandoning this. At the devmeeting it was agreed that 'getPrimitiveSizeInBits' should continue to work as-is for fixed-length vectors and only behave differently for scalable vectors.

Indeed, we shouldn't allow scalable vectors to be globals. I've added a check for that in the verifier, plus unit tests and a small update to the langref. Thanks.

I think the discussion on the mailing list has reached agreement on this type being suitable for both SVE and RVV; any review comments on the code or tests?

Fixed string representation of scalable vectors which don't map to the existing defined set of MVTs.

Removed unnecessary metadata.

Updated RFC including a list of patches for simple codegen using this extension has been posted: http://lists.llvm.org/pipermail/llvm-dev/2018-June/123780.html