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

[Eli Friedman via Phabricator via cfe-commits]

efriedma marked 5 inline comments as done.
efriedma added a comment.

In D72467#1838591 <https://reviews.llvm.org/D72467#1838591>, @spatel wrote:

> LGTM, but should get a 2nd opinion since I'm not familiar with some of the parts.


Any specific part you're worried about?



================
Comment at: llvm/include/llvm/IR/Instructions.h:1980
 
+constexpr int UndefMaskElem = -1;
+
----------------
sdesmalen wrote:
> nit: do we want to make this a member of ShuffleVectorInst, as this is likely the only context in which it will be used?
ShuffleVectorInst::UndefMaskElem is sort of long. Maybe we don't use it in enough places to matter.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:3597
 
-  if (MaskV->isNullValue() && VT.isScalableVector()) {
+  if (all_of(Mask, [](int Elem) { return Elem == 0; }) &&
+      VT.isScalableVector()) {
----------------
sdesmalen wrote:
> Should this use `m_ZeroMask()` ?
I don't want to make unrelated functional changes in this patch.


================
Comment at: llvm/lib/IR/ConstantFold.cpp:870
   // Undefined shuffle mask -> undefined value.
-  if (isa<UndefValue>(Mask))
+  if (all_of(Mask, [](int Elt) { return Elt == UndefMaskElem; }))
     return UndefValue::get(VectorType::get(EltTy, MaskNumElts));
----------------
sdesmalen wrote:
> Is it worth adding a `m_UndefMask` ?
We currently only use this pattern in three places, and I doubt we're going to add more.


================
Comment at: llvm/lib/IR/Constants.cpp:2260
+  Constant *ArgVec[] = { V1, V2 };
+  ConstantExprKeyType Key(Instruction::ShuffleVector, ArgVec, 0, 0, None, Mask);
 
----------------
sdesmalen wrote:
> nit: is there a reason for dropping `const` here?
Doesn't really matter either way, but we generally don't add const markings to local variables just because we can.


================
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,
----------------
ctetreau wrote:
> 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.
I'd prefer to just say the result is scalable if and only if the operand is scalable, at least for now.  It's not clear what semantics we want for a fixed shuffle of a scalable operand, and allowing scalable splats of fixed vectors doesn't really allow expressing anything new.  We can relax it later once we've settled on our general approach to scalable shuffles.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72467