[PATCH] D70093: [InstCombine] Avoid moving ops that do restrict undef across shuffles.

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:1513
+    }
+  };
+
----------------
RKSimon wrote:
> @spatel Do we have an existing helper for something like this? I know you added getKnownUndefForVectorBinop to the DAG
I don't think there's a twin of that for IR. Unfortunately, we have bits and pieces of the puzzle in both places...


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:1568
       //       for target-independent shuffle creation.
       if (I >= SrcVecNumElts) {
         Constant *MaybeUndef =
----------------
What if we extend this check instead:
      if (I >= SrcVecNumElts || ShMask[I] < 0) {

Currently, we're transferring an undef element from the original shuffle to the newly created shuffle, but that's not safe in general. It is safe, however, if the binop would constant fold to undef given an undef operand.

(Sorry this code got so complicated...)


================
Comment at: llvm/test/Transforms/InstCombine/vec_shuffle.ll:1010-1016
-; CHECK-NEXT:    [[AND:%.*]] = shufflevector <4 x i16> [[ADD:%.*]], <4 x i16> undef, <4 x i32> <i32 undef, i32 undef, i32 1, i32 1>
+; CHECK-NEXT:    [[SHUFFLE:%.*]] = shufflevector <4 x i16> [[ADD:%.*]], <4 x i16> undef, <4 x i32> <i32 undef, i32 undef, i32 1, i32 1>
+; CHECK-NEXT:    [[AND:%.*]] = and <4 x i16> [[SHUFFLE]], <i16 0, i16 0, i16 -1, i16 -1>
 ; CHECK-NEXT:    ret <4 x i16> [[AND]]
 ;
 entry:
   %shuffle = shufflevector <4 x i16> %add, <4 x i16> undef, <4 x i32> <i32 undef, i32 undef, i32 1, i32 1>
   %and = and <4 x i16> %shuffle, <i16 0, i16 0, i16 -1, i16 -1>
   ret <4 x i16> %and
----------------
lebedev.ri wrote:
> Hmm yes, i'd agree this is a miscompile..
This test is not committed to trunk? Add that along with similar patterns for other binop opcodes like the add/sub/shl noted in this patch? That way we'll know if this or my alternate suggestion is behaving as intended.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70093