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