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

[Rackover, Zvi via llvm-commits]

Hi Andrea,

Your suggestion makes a lot of sense, and I too had similar thoughts about the benefits of moving logic from InstCombine to InstructionSimplify, and the benefits of canonicalizing early. ‘foldOrCommuteConstant’ is an example of how early canonicalization is done on binary operations to allow assumption to be held – we should definitely have a mechanism for shuffles.
I was planning on first adding a few shuffle simplifications which will show how badly we need to canonicalize early to avoid duplications and redundancies, and follow with some canonicalization and refactoring as NFC changes. Then start another iteration of adding more cases that can be handled by SimplifyShuffleVectorInst, followed by more cleanup, and so on.

Would appreciate feedback and comments.

Thanks, Zvi

From: Andrea Di Biagio [mailto:andrea.dibiagio at gmail.com]
Sent: Tuesday, April 04, 2017 17:11
To: Rackover, Zvi <zvi.rackover at intel.com>
Cc: llvm-commits <llvm-commits at lists.llvm.org>
Subject: Re: [llvm] r299412 - InstCombine: Use the InstSimplify hook for shufflevector

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<mailto: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<mailto:llvm-commits at lists.llvm.org>
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits

---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170404/dccc184d/attachment.html>