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

[spatel]

Summary

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

We replace undefs with a safe constant (0 or -1) based on the (non-)negative
predicate constraint.

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

Previously, we were relying on getSafeVectorConstantForBinop() in the related fold (D76800).
But that's making an assumption about what qualifies as "safe", and that assumption may
not always hold.

[lebedev.ri]

This LG.

[spatel]

Thinking about this again...assuming that we know what getSafeVectorConstantForBinop() is going to return is dirty. Let's just replace undefs with the values we want.

[spatel]

Patch updated:
Use replaceUndefsWith() directly with 0/-1 constants, so we're not implicitly assuming the behavior of the APIs.

[lebedev.ri]

I suspect there are other cases like this (where we used getSafeVectorConstantForBinop()).

[spatel]

In D77739#1970300, @lebedev.ri wrote:

I suspect there are other cases like this (where we used getSafeVectorConstantForBinop()).

I did a quick check of the remaining (3) uses, and they look ok to me. Those are shuffle+binop transforms and are not gated by a constraint on the constant values (other than we don't want to create extra UB/poison via re-ordering).

llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp

if (match(&I, m_Not(m_BinOp(NotVal)))) {
     // the 'not' by inverting the constant and using the opposite shift type.
     // Canonicalization rules ensure that only a negative constant uses 'ashr',
     // but we must check that in case that transform has not fired yet.
 
     // ~(C >>s Y) --> ~C >>u Y (when inverting the replicated sign bits)
     Constant *C;
     if (match(NotVal, m_AShr(m_Constant(C), m_Value(Y))) &&
         match(C, m_Negative())) {
-      Constant *NewC = ConstantExpr::getNot(C);
-      if (C->getType()->isVectorTy())
-        NewC = getSafeVectorConstantForBinop(Instruction::LShr, NewC, false);
-      return BinaryOperator::CreateLShr(NewC, Y);
+      // We matched a negative constant, so propagating undef is unsafe.
+      // Clamp undef elements to -1.
+      Type *EltTy = C->getType()->getScalarType();
+      C = Constant::replaceUndefsWith(C, ConstantInt::getAllOnesValue(EltTy));
+      return BinaryOperator::CreateLShr(ConstantExpr::getNot(C), Y);
     }
 
     // ~(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()))
+        match(C, m_NonNegative())) {
+      // We matched a non-negative constant, so propagating undef is unsafe.
+      // Clamp undef elements to 0.
+      Type *EltTy = C->getType()->getScalarType();
+      C = Constant::replaceUndefsWith(C, ConstantInt::getNullValue(EltTy));
       return BinaryOperator::CreateAShr(ConstantExpr::getNot(C), Y);
+    }
 
     // ~(X + C) --> -(C + 1) - X
     if (match(Op0, m_Add(m_Value(X), m_Constant(C))))
       return BinaryOperator::CreateSub(ConstantExpr::getNeg(AddOne(C)), X);
   }
 
   // Use DeMorgan and reassociation to eliminate a 'not' op.
   Constant *C1;

llvm/test/Transforms/InstCombine/vector-xor.ll

 ;
   %1 = lshr <4 x i32> <i32  3, i32  5, i32  7, i32  9>, %a0
   %2 = xor  <4 x i32> <i32 -1, i32 -1, i32 -1, i32 -1>, %1
   ret <4 x i32> %2
 }
 
 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
   %2 = xor  <4 x i32> <i32 -1, i32 -1, i32 -1, i32 undef>, %1
   ret <4 x i32> %2
 }
 
 ; ~(C-X) == X-C-1 == X+(-C-1)