[PATCH] D77739: [InstCombine] replace undef in vector constant for safe shift transform (PR45447)

[Sanjay Patel via Phabricator via llvm-commits]

spatel created this revision.
spatel added reviewers: lebedev.ri, nlopes, jroelofs.
Herald added subscribers: hiraditya, mcrosier.
lebedev.ri accepted this revision.
lebedev.ri added a comment.
This revision is now accepted and ready to land.

This LG.


As noted in PR45447, we have a vector-constant-with-undef-element transform bug: 
https://bugs.llvm.org/show_bug.cgi?id=45447

The fix to use getSafeVectorConstantForBinop() is familiar at this point (although we missed catching this with the related bug in D76800 <https://reviews.llvm.org/D76800>). But there's a logic difference caused by the match that guards the transform.

So this is correct:
http://volta.cs.utah.edu:8080/z/WZE36H
...but this is not:
http://volta.cs.utah.edu:8080/z/boj8gJ

I didn't see a good way to combine the logic for these 2 folds, but if you do, let me know.


https://reviews.llvm.org/D77739

Files:
  llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
  llvm/test/Transforms/InstCombine/vector-xor.ll


Index: llvm/test/Transforms/InstCombine/vector-xor.ll
===================================================================
--- llvm/test/Transforms/InstCombine/vector-xor.ll
+++ llvm/test/Transforms/InstCombine/vector-xor.ll
@@ -172,7 +172,7 @@
 
 define <4 x i32> @test_v4i32_not_lshr_nonnegative_const_undef(<4 x i32> %a0) {
 ; CHECK-LABEL: @test_v4i32_not_lshr_nonnegative_const_undef(
-; CHECK-NEXT:    [[TMP1:%.*]] = ashr <4 x i32> <i32 -4, i32 -6, i32 undef, i32 -10>, [[A0:%.*]]
+; CHECK-NEXT:    [[TMP1:%.*]] = ashr <4 x i32> <i32 -4, i32 -6, i32 -1, i32 -10>, [[A0:%.*]]
 ; CHECK-NEXT:    ret <4 x i32> [[TMP1]]
 ;
   %1 = lshr <4 x i32> <i32  3, i32  5, i32 undef, i32  9>, %a0
Index: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
===================================================================
--- llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
+++ llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
@@ -3071,6 +3071,10 @@
     Constant *C;
     if (match(NotVal, m_AShr(m_Constant(C), m_Value(Y))) &&
         match(C, m_Negative())) {
+      // There's a correctness subtlety here: we matched a negative constant,
+      // but the safe constant LHS for a shift is returned as zero, so it does
+      // not conform with the negative predicate constraint. Get the 'not'
+      // constant and then replace any undef elements with 0.
       Constant *NewC = ConstantExpr::getNot(C);
       if (C->getType()->isVectorTy())
         NewC = getSafeVectorConstantForBinop(Instruction::LShr, NewC, false);
@@ -3079,8 +3083,15 @@
 
     // ~(C >>u Y) --> ~C >>s Y (when inverting the replicated sign bits)
     if (match(NotVal, m_LShr(m_Constant(C), m_Value(Y))) &&
-        match(C, m_NonNegative()))
-      return BinaryOperator::CreateAShr(ConstantExpr::getNot(C), Y);
+        match(C, m_NonNegative())) {
+      // Unlike the similar pattern above, we matched a non-negative constant,
+      // so the safe constant LHS for a shift ("0") is ok. So we replace undef
+      // first, then 'not' that to create a safe and correct constant operand.
+      if (C->getType()->isVectorTy())
+        C = getSafeVectorConstantForBinop(Instruction::LShr, C, false);
+      Constant *NewC = ConstantExpr::getNot(C);
+      return BinaryOperator::CreateAShr(NewC, Y);
+    }
 
     // ~(X + C) --> -(C + 1) - X
     if (match(Op0, m_Add(m_Value(X), m_Constant(C))))


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D77739.256040.patch
Type: text/x-patch
Size: 2394 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200408/09720598/attachment.bin>