Superseded by D66302

Superseded by D65448

Superseded by D68203

• Removed const qualifier from queries
• Swapped to std::make_tuple for the comparisons in CodeGenDAGPatterns.cpp

I also changed the SelectionDAGBuilder code for masked loads/stores/gathers/scatters to use the known min size when creating a MachineMemoryOperand, since MMO isn't aware of scalable types yet. I've left TODOs as reminders.

• Switched the lambdas in CodeGenDAGPatterns to use std::tie for ordering. This needed the TypeSize objects to be constant (I get a "non-const lvalue reference" error otherwise), so I changed all query methods to return const objects.
• Changed stack offset to deliberately use a TypeSize. This hit another problem with implicit promotion and signed v. unsigned casting, so I've added explicit casts.

In D68476#1725466, @mgudim wrote:

Renamed the inrinsics according to the convention.

• Refactored to match the same approach used in D53137 -- TypeSize returned for all size queries instead of introducing separate interfaces and relying on implicit conversion to be (mostly) compatible with existing code.
• Added 'isByteSized()' interface to TypeSize.
• Minor bugfix to DAGCombiner::ForwardStoreValueToDirectLoad, where implicit conversions allowed an unsigned number to wrap and then be converted to a larger signed integer; with TypeSize using uint64_t instead, this showed up during a make check-all run with ubsan active. Can't write a test for that one since the transform bailed out when Offset was non-zero.

I was also expecting something that says "setOperationAction(ISD::MLOAD, VT, Legal)" somewhere, but I guess that's already the default?

Hi, thanks for waiting a bit. LGTM.

• 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.