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

Sander de Smalen via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Jan 24 06:59:05 PST 2020


sdesmalen added a comment.

Thanks @efriedma for working on this. The overall approach seems good to me!
Mostly added some nits, with the exception of one question on what to do with a shufflevector with fixed-width mask and scalable source vectors.



================
Comment at: llvm/include/llvm/IR/Instructions.h:1980
 
+constexpr int UndefMaskElem = -1;
+
----------------
nit: do we want to make this a member of ShuffleVectorInst, as this is likely the only context in which it will be used?


================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:2694
     pushValue(I.getOperand(1), InstID, Vals);
-    pushValue(I.getOperand(2), InstID, Vals);
+    pushValue(cast<ShuffleVectorInst>(I).getShuffleMaskForBitcode(), InstID, Vals);
     break;
----------------
nit: >80char (please run through clang-format)


================
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()) {
----------------
Should this use `m_ZeroMask()` ?


================
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));
----------------
Is it worth adding a `m_UndefMask` ?


================
Comment at: llvm/lib/IR/Constants.cpp:2260
+  Constant *ArgVec[] = { V1, V2 };
+  ConstantExprKeyType Key(Instruction::ShuffleVector, ArgVec, 0, 0, None, Mask);
 
----------------
nit: is there a reason for dropping `const` here?


================
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,
----------------
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.


================
Comment at: llvm/lib/IR/ConstantsContext.h:470
 struct ConstantExprKeyType {
+private:
   uint8_t Opcode;
----------------
nit: make ConstantExprKeyType a `class`?


================
Comment at: llvm/lib/IR/Instructions.cpp:1948
+  ShuffleMaskForBitcode = convertShuffleMaskForBitcode(Mask, getType());
+
+}
----------------
nit: odd newline


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72467





More information about the llvm-commits mailing list