[PATCH] D72467: Remove "mask" operand from shufflevector.

[Christopher Tetreault via Phabricator via llvm-commits]

ctetreau added inline comments.


================
Comment at: llvm/lib/IR/ConstantsContext.h:153
                    cast<VectorType>(C1->getType())->getElementType(),
-                   cast<VectorType>(C3->getType())->getElementCount()),
+                   Mask.size(), C1->getType()->getVectorIsScalable()),
                  Instruction::ShuffleVector,
----------------
sdesmalen wrote:
> The number of elements in the result matches the number of elements in the mask, but if we assume 'scalable' from one of the source vectors, this means we make the choice that we cannot extract a fixed-width vector from a scalable vector, e.g.
> 
>   shufflevector(<vscale x 4 x i32> %in, <vscale x 4 x i32> undef, <4 x i32> <i32 0, i32 1, i32 2, i32 3>)
> 
> The LangRef does not explicitly call out this case (and it currently fails to compile), but it would provide a way to extract a sub-vector from a scalable vector.
> If we want to allow this, we'd also need to decide what the meaning would be of:
> 
>   shufflevector(<vscale x 4 x i32> %in, <vscale x 4 x i32> undef, <4 x i32> <i32 5, i32 6, i32 7, i32 8>)
> 
> which may not be valid if `vscale = 1`.
> 
> Alternatively, we could implement this with an additional extract intrinsic and add the restriction that if the source values are scalable, the mask must be scalable as well. If so, then we should update the LangRef to explicitly disallow the above case.
For the time being, Either this assumption must hold, or a bool must be added that is true if the mask is from a scalable vector.

There are places in the codebase that assume that the mask has a concrete value known at compile time. These sites are currently the sites of bugs, and the fix is to not try to access the mask if the vector is scalable. We need some way of knowing that the mask is scalable, either by the assumption made here, or a queryable bool value.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72467/new/

https://reviews.llvm.org/D72467