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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
