[PATCH] D27101: [Type] Extend VectorType to support scalable vectors. [IR support for SVE scalable vectors 1/4]

[Renato Golin via Phabricator via llvm-commits]

rengolin added a comment.

Hi Paul,

I have a few inline comments, but there are two huge questions about this patch:

1. Is this the right move?

This will be answered by a more thorough review on all the possibilities, representations and future expansions of the IR. I haven't spotted any backwards incompatibility, but I could be wrong. There is the obvious forwards incompatibility, which is less serious but non-trivial.

I'll let people comment on that, but from my side, the new representation looks good for what's intended.

2. Can we do it now?

With the release 4.0 branching off any time in December, we may be asked to hold off big IR changes if they have any visible impact on everyone else's work. Your `PackedEC` representation is a big one, because it will produce large values for those IR readers that don't understand it (ie. all projects that follow releases or old trunk and have to interact with upstream IR).

I don't know off the top of my head which problems they are or who has them, so we'll have to wait for people to shed some light on that, here.

Finally, tests. I'm surprised if those two new lines are **all** we have on vector parsing, printing and validating. I don't really know where the tests are, but we probably have more than that. We'll need tests parsing different types of vectors, different types of failures (like <4 x n x i32>), etc. Also, make sure to include an example of scalable vectors on each new LangRef topic.

cheers,
--renato



================
Comment at: docs/LangRef.rst:6722
+      <result> = extractelement <M x <ty>> <val>, <ty2> <idx>      ; yields <ty>
+      <result> = extractelement <n x M x <ty>> <val>, <ty2> <idx>  ; yields <ty>
 
----------------
For purely notational reasons, we may want to keep the "n" as the primary multiplier, since it's already widely commented and documented across the code and docs. Pick another unambiguous constant symbol, like "k", or even "m" (from "multiples" of vectors).


================
Comment at: docs/LangRef.rst:6806
+      <result> = shufflevector <M x <ty>> <v1>, <M x <ty>> <v2>, <M2 x i32> <mask>                ; yields <M2 x <ty>>
+      <result> = shufflevector <n x M x <ty>> <v1>, <n x M x <ty>> <v2>, <n x M2 x i32> <mask>    ; yields <n x M2 x <ty>>
 
----------------
I'm uncertain about this one. How will the masks look like?

Say, for instance, on a zip operation. On normal vectors, it's something like:

  %tmp = shufflevector <4 x i32> %v1, <4 x i32> %v2,
                       <8 x i32> <i32 0, i32 4, i32 1, i32 5, i32 2, i32 6, i32 3, i32 7>
  %newv1 = shufflevector <8 x i32> %tmp, <8 x i32> undef,
                         <4 x i32> <i32 0, i32 1, i32 2, i32 3>
  %newv2 = shufflevector <8 x i32> %tmp, <8 x i32> undef,
                         <4 x i32> <i32 4, i32 5, i32 6, i32 7>

and can only be represented in IR because the length is known at compile time. Anything based on "vscale" will be a really complicated pattern and won't work for all cases even if some of them are simple.


================
Comment at: include/llvm/IR/DerivedTypes.h:368
+public:
+  /// A VectorType is of the form <N x M x Ty>.  M is the minimum number of
+  /// elements of type Ty contained within the vector, with the actual element
----------------
Same here, keep the old notation to avoid mismatch with current comments.


================
Comment at: include/llvm/IR/DerivedTypes.h:372
+  /// or guaranteed to be one.  For the latter case the extra complication is
+  /// discarded leading to:
+  ///
----------------
You mention the "latter" but gives an example of each. I think you can stop at "discarded", then mention examples below.


================
Comment at: include/llvm/IR/DerivedTypes.h:419
   /// This static method is the primary way to construct an VectorType.
-  static VectorType *get(Type *ElementType, unsigned NumElements);
+  static VectorType *get(Type *ElType, ElementCount EC);
+  static VectorType *get(Type *ElType, unsigned NumEls, bool Scalable=false) {
----------------
I don't think ElementCount should be exposed to the user. The constructor below is essentially identical.


================
Comment at: lib/IR/Type.cpp:641
 
+  unsigned PackedEC = EC.getMinNumElements() | ((unsigned)EC.isScalable()<<31);
   LLVMContextImpl *pImpl = ElementType->getContext().pImpl;
----------------
`unsigned int` is not guaranteed to be 32-bits


https://reviews.llvm.org/D27101