[llvm] r299412 - InstCombine: Use the InstSimplify hook for shufflevector

[Andrea Di Biagio via llvm-commits]

Hi Zvi,

thanks for this patch.

I noticed that method visitShuffleVectorInst() knows already how to
simplify shuffles (using `SimplifyDemandedVectorElts()`) and canonicalize
shuffles according to the following rules:

  // Canonicalize shuffle(x    ,x,mask) -> shuffle(x, undef,mask')
  // Canonicalize shuffle(undef,x,mask) -> shuffle(x, undef,mask').

SimplifyDemandedVectorElts() would be able to identify shuffles where only
one operands is used. That means, SimplifyDemandedVectorElts() is able to
propagate Undef to unused operands of a shuffle.

I think that the logic in SimplifyShuffleVectorInst() could be much simpler
if only we could assume that shuffles have been already canonicalized and
simplified using `SimplifyDemandedVectorElts()`.
If we assume canonical shuffles in input, then the following scenario can
never happen:

  // If only one of the operands is constant, constant fold the shuffle if
the
  // mask does not select elements from the variable operand.

That is because 1) SimplifyDemandedVectorElts() would have replaced the
unused operand with Undef, and 2) the Undef would always be the second
operand.

The question is: should we consider moving all the shuffle simplification
logic from InstCombineVectorOps.cpp to InstructionSimplify.cpp?

I am under the impression that all that logic should live in
InstructionSimplify.cpp.
It would also help simplifying the logic in SimplifyShuffleVectorInst() (we
would only need the first rule of that function which knows how to fold a
shuffle with operands that are both constant vectors/undef).

Not sure if this all makes sense, but I hope it helps improving that code.

-Andrea


On Tue, Apr 4, 2017 at 5:47 AM, Zvi Rackover via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> Author: zvi
> Date: Mon Apr  3 23:47:57 2017
> New Revision: 299412
>
> URL: http://llvm.org/viewvc/llvm-project?rev=299412&view=rev
> Log:
> InstCombine: Use the InstSimplify hook for shufflevector
>
> Summary: Start using the recently added InstSimplify hook for shuffles in
> the respective InstCombine visitor.
>
> Reviewers: spatel, RKSimon, craig.topper, majnemer
>
> Reviewed By: majnemer
>
> Subscribers: majnemer, llvm-commits
>
> Differential Revision: https://reviews.llvm.org/D31526
>
> Modified:
>     llvm/trunk/lib/Transforms/InstCombine/InstCombineVectorOps.cpp
>
> Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombineVectorOps.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/
> Transforms/InstCombine/InstCombineVectorOps.cpp?rev=
> 299412&r1=299411&r2=299412&view=diff
> ============================================================
> ==================
> --- llvm/trunk/lib/Transforms/InstCombine/InstCombineVectorOps.cpp
> (original)
> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineVectorOps.cpp Mon
> Apr  3 23:47:57 2017
> @@ -1140,12 +1140,11 @@ Instruction *InstCombiner::visitShuffleV
>    SmallVector<int, 16> Mask = SVI.getShuffleMask();
>    Type *Int32Ty = Type::getInt32Ty(SVI.getContext());
>
> -  bool MadeChange = false;
> -
> -  // Undefined shuffle mask -> undefined value.
> -  if (isa<UndefValue>(SVI.getOperand(2)))
> -    return replaceInstUsesWith(SVI, UndefValue::get(SVI.getType()));
> +  if (auto *V = SimplifyShuffleVectorInst(LHS, RHS, SVI.getMask(),
> +                                          SVI.getType(), DL, &TLI, &DT,
> &AC))
> +    return replaceInstUsesWith(SVI, V);
>
> +  bool MadeChange = false;
>    unsigned VWidth = SVI.getType()->getVectorNumElements();
>
>    APInt UndefElts(VWidth, 0);
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170404/71268e2e/attachment.html>