[InstSimplify/NewGVN] Add option to control the use of undef.

[fhahn]

Summary

Making use of undef is not safe if the simplification result is not used
to replace all uses of the result. This leads to problems in NewGVN,
which does not replace all uses in the IR directly. See PR33165 for more
details.

This patch adds an option to SimplifyQuery to disable the use of undef.

Note that I've only guarded uses if isa<UndefValue>/m_Undef where
SimplifyQuery is currently available. If we agree on the general
direction, I'll update the remaining uses.

[efriedma]

Is the goal here that it's legal to replace the output value of SimplifyInstruction with the input value, in addition to the normal replacement of the input value with the output value? Or do we specifically care about literal UndefValue constants somehow, as opposed to values which are undef? If we had a PoisonValue constant, would we also need to exclude it?

[fhahn]

In D84792#2179916, @efriedma wrote:

Is the goal here that it's legal to replace the output value of SimplifyInstruction with the input value, in addition to the normal replacement of the input value with the output value? Or do we specifically care about literal UndefValue constants somehow, as opposed to values which are undef? If we had a PoisonValue constant, would we also need to exclude it?

I think we probably want to be able to replace in both directions as you mentioned. I should have been more specific. The immediate concern is undef constants, as they are most likely to introduce problems (e.g. if the the undef is interpreted as different value for different simplify calls). But we probably also want to limit distribution, as in @aqjune 's example in D84655. This is probably going to be much harder to audit for though I am afraid.

[efriedma]

Do we also need to restrict transforms like "X & 0 = 0"?

[aqjune]

I'm afraid that adding Q.CanUseUndef to the places where isa<UndefValue> is used isn't enough; equality (==) can implicitly fold undef since UndefValue is singleton.

For example, given icmp eq X, Y, if X and Y are both UndefValue, comparison X == Y will yield true. This will trigger this transformation even if Q.CanUseUndef was false:

// icmp X, X -> true/false
// icmp X, undef -> true/false because undef could be X.
if (LHS == RHS || (Q.CanUseUndef && isa<UndefValue>(RHS)))
  return ConstantInt::get(ITy, CmpInst::isTrueWhenEqual(Pred));

However, in theory, this is also using one of the two undefs by constraining it to have a value that is equal to another undef.

I'm just brainstorming, what if there is a pass that removes all undefs from a function by folding them away? If there is no undef, bad undef foldings will never happen trivially. InstSimplify may create undefs on-the-fly again, but making InstSimplify never create undef will be easier.

[nikic]

Is the goal here that it's legal to replace the output value of SimplifyInstruction with the input value, in addition to the normal replacement of the input value with the output value? Or do we specifically care about literal UndefValue constants somehow, as opposed to values which are undef? If we had a PoisonValue constant, would we also need to exclude it?

I think the only thing we need to prevent is that InstSimplify assumes a specific value for (or any kind of constraint on) an undef value. We can still allow other refinement, and I don't think poison values are problematic in this context either, because they don't have a consistency requirement (we don't have to pick one particular value for the poison -- and generally can't).

Do we also need to restrict transforms like "X & 0 = 0"?

Same here, this should not be a problem. Even if X is undef, this transform is valid for any choice of value for X.

For example, given icmp eq X, Y, if X and Y are both UndefValue, comparison X == Y will yield true.

Hm, this is a tricky case. I agree that doing this fold is not right, but it also seems hard to really enforce, because of how many value comparisons there are (and not all of them are problematic). I think it may be better to ignore this particular case until we have evidence that it can cause issues in practice (the cases where this could occur are a lot more narrow, because at the very least it involves two undef values from distinct sources).

[fhahn]

In D84792#2182195, @nikic wrote:

Is the goal here that it's legal to replace the output value of SimplifyInstruction with the input value, in addition to the normal replacement of the input value with the output value? Or do we specifically care about literal UndefValue constants somehow, as opposed to values which are undef? If we had a PoisonValue constant, would we also need to exclude it?

I think the only thing we need to prevent is that InstSimplify assumes a specific value for (or any kind of constraint on) an undef value. We can still allow other refinement, and I don't think poison values are problematic in this context either, because they don't have a consistency requirement (we don't have to pick one particular value for the poison -- and generally can't).

Yeah as mentioned earlier, assuming different values for undef constants is the biggest issue in practice. Thinking about it again, requiring replacement in both directions would be too limiting due to undef (e.g. we cannot replace a constant with an instruction, unless we know none of the operands are poison).

Hm, this is a tricky case. I agree that doing this fold is not right, but it also seems hard to really enforce, because of how many value comparisons there are (and not all of them are problematic). I think it may be better to ignore this particular case until we have evidence that it can cause issues in practice (the cases where this could occur are a lot more narrow, because at the very least it involves two undef values from distinct sources).

Yes I think we would have to accept that CanUseUndef is 'best-effort' and incomplete. Not great, but I don't think there's any practical way to provide a complete implementation.

[aqjune]

Okay. I agree that it may not be problematic in practice; if it matters, icmps with two arguments being undef can be detected in advance and folded.

[fhahn]

Ping. Should we go with this approach to start with, to unblock some patches and iterate on the details as follow-ups? It's not ideal for CanUseUndef to not cover all cases initially, but I am not sure what the alternative would be.

[nikic]

In D84792#2197183, @fhahn wrote:

Ping. Should we go with this approach to start with, to unblock some patches and iterate on the details as follow-ups? It's not ideal for CanUseUndef to not cover all cases initially, but I am not sure what the alternative would be.

Yes, that SGTM.

llvm/include/llvm/Analysis/InstructionSimplify.h

103

Possibly be a bit more specific here and say that simplifications that constrain the range of possible values for an undef are not allowed.

[fhahn]

Adjust wording of option to mention that simplifications are not allowed to constrain the range of value for uses of undef. Example is that simplifications cannot assume a certain value for a use of undef.

llvm/include/llvm/Analysis/InstructionSimplify.h

struct SimplifyQuery {
   AssumptionCache *AC = nullptr;
   const Instruction *CxtI = nullptr;
 
   // Wrapper to query additional information for instructions like metadata or
   // keywords like nsw, which provides conservative results if those cannot
   // be safely used.
   const InstrInfoQuery IIQ;
 
+  /// Controls whether simplifications are allowed to constrain the range of
+  /// possible values for uses of undef. If it is false, simplifications are not
+  /// allowed to assume a particular value for a use of undef for example.
+  bool CanUseUndef;
+
   SimplifyQuery(const DataLayout &DL, const Instruction *CXTI = nullptr)
       : DL(DL), CxtI(CXTI) {}
 
   SimplifyQuery(const DataLayout &DL, const TargetLibraryInfo *TLI,
                 const DominatorTree *DT = nullptr,
                 AssumptionCache *AC = nullptr,
-                const Instruction *CXTI = nullptr, bool UseInstrInfo = true)
-      : DL(DL), TLI(TLI), DT(DT), AC(AC), CxtI(CXTI), IIQ(UseInstrInfo) {}
+                const Instruction *CXTI = nullptr, bool UseInstrInfo = true,
+                bool CanUseUndef = true)
+      : DL(DL), TLI(TLI), DT(DT), AC(AC), CxtI(CXTI), IIQ(UseInstrInfo),
+        CanUseUndef(CanUseUndef) {}
   SimplifyQuery getWithInstruction(Instruction *I) const {
     SimplifyQuery Copy(*this);
     Copy.CxtI = I;
     return Copy;
   }
 };
 
 // NOTE: the explicit multiple argument versions of these functions are

llvm/lib/Analysis/InstructionSimplify.cpp

static Value *ThreadBinOpOverSelect(Instruction::BinaryOps Opcode, Value *LHS,
   }
 
   // If they simplified to the same value, then return the common value.
   // If they both failed to simplify then return null.
   if (TV == FV)
     return TV;
 
   // If one branch simplified to undef, return the other one.
-  if (TV && isa<UndefValue>(TV))
+  if (TV && Q.CanUseUndef && isa<UndefValue>(TV))
     return FV;
-  if (FV && isa<UndefValue>(FV))
+  if (FV && Q.CanUseUndef && isa<UndefValue>(FV))
     return TV;
 
   // If applying the operation did not change the true and false select values,
   // then the result of the binop is the select itself.
   if (TV == SI->getTrueValue() && FV == SI->getFalseValue())
     return SI;
 
   // If one branch simplified and the other did not, and the simplified

 /// Given operands for an Add, see if we can fold the result.
 /// If not, this returns null.
 static Value *SimplifyAddInst(Value *Op0, Value *Op1, bool IsNSW, bool IsNUW,
                               const SimplifyQuery &Q, unsigned MaxRecurse) {
   if (Constant *C = foldOrCommuteConstant(Instruction::Add, Op0, Op1, Q))
     return C;
 
   // X + undef -> undef
-  if (match(Op1, m_Undef()))
+  if (Q.CanUseUndef && match(Op1, m_Undef()))
     return Op1;
 
   // X + 0 -> X
   if (match(Op1, m_Zero()))
     return Op0;
 
   // If two operands are negative, return 0.
   if (isKnownNegation(Op0, Op1))

 /// If not, this returns null.
 static Value *SimplifyMulInst(Value *Op0, Value *Op1, const SimplifyQuery &Q,
                               unsigned MaxRecurse) {
   if (Constant *C = foldOrCommuteConstant(Instruction::Mul, Op0, Op1, Q))
     return C;
 
   // X * undef -> 0
   // X * 0 -> 0
-  if (match(Op1, m_CombineOr(m_Undef(), m_Zero())))
+  if (Q.CanUseUndef && match(Op1, m_CombineOr(m_Undef(), m_Zero())))
     return Constant::getNullValue(Op0->getType());
 
   // X * 1 -> X
   if (match(Op1, m_One()))
     return Op0;
 
   // (X / Y) * Y -> X if the division is exact.
   Value *X = nullptr;
if (Value *V = SimplifyShift(Opcode, Op0, Op1, Q, MaxRecurse))
     return V;
 
   // X >> X -> 0
   if (Op0 == Op1)
     return Constant::getNullValue(Op0->getType());
 
   // undef >> X -> 0
   // undef >> X -> undef (if it's exact)
-  if (match(Op0, m_Undef()))
+  if (Q.CanUseUndef && match(Op0, m_Undef()))
     return isExact ? Op0 : Constant::getNullValue(Op0->getType());
 
   // The low bit cannot be shifted out of an exact shift if it is set.
   if (isExact) {
     KnownBits Op0Known = computeKnownBits(Op0, Q.DL, /*Depth=*/0, Q.AC, Q.CxtI, Q.DT);
     if (Op0Known.One[0])
       return Op0;
   }
 
   return nullptr;
 }
 
 /// Given operands for an Shl, see if we can fold the result.
 /// If not, this returns null.
 static Value *SimplifyShlInst(Value *Op0, Value *Op1, bool isNSW, bool isNUW,
                               const SimplifyQuery &Q, unsigned MaxRecurse) {
   if (Value *V = SimplifyShift(Instruction::Shl, Op0, Op1, Q, MaxRecurse))
     return V;
 
   // undef << X -> 0
   // undef << X -> undef if (if it's NSW/NUW)
-  if (match(Op0, m_Undef()))
+  if (Q.CanUseUndef && match(Op0, m_Undef()))
     return isNSW || isNUW ? Op0 : Constant::getNullValue(Op0->getType());
 
   // (X >> A) << A -> X
   Value *X;
   if (Q.IIQ.UseInstrInfo &&
       match(Op0, m_Exact(m_Shr(m_Value(X), m_Specific(Op1)))))
     return X;
 

 /// Given operands for an And, see if we can fold the result.
 /// If not, this returns null.
 static Value *SimplifyAndInst(Value *Op0, Value *Op1, const SimplifyQuery &Q,
                               unsigned MaxRecurse) {
   if (Constant *C = foldOrCommuteConstant(Instruction::And, Op0, Op1, Q))
     return C;
 
   // X & undef -> 0
-  if (match(Op1, m_Undef()))
+  if (Q.CanUseUndef && match(Op1, m_Undef()))
     return Constant::getNullValue(Op0->getType());
 
   // X & X = X
   if (Op0 == Op1)
     return Op0;
 
   // X & 0 = 0
   if (match(Op1, m_Zero()))

 static Value *SimplifyOrInst(Value *Op0, Value *Op1, const SimplifyQuery &Q,
                              unsigned MaxRecurse) {
   if (Constant *C = foldOrCommuteConstant(Instruction::Or, Op0, Op1, Q))
     return C;
 
   // X | undef -> -1
   // X | -1 = -1
   // Do not return Op1 because it may contain undef elements if it's a vector.
-  if (match(Op1, m_Undef()) || match(Op1, m_AllOnes()))
+  if ((Q.CanUseUndef && match(Op1, m_Undef())) || match(Op1, m_AllOnes()))
     return Constant::getAllOnesValue(Op0->getType());
 
   // X | X = X
   // X | 0 = X
   if (Op0 == Op1 || match(Op1, m_Zero()))
     return Op0;
 
   // A | ~A  =  ~A | A  =  -1

 /// Given operands for a Xor, see if we can fold the result.
 /// If not, this returns null.
 static Value *SimplifyXorInst(Value *Op0, Value *Op1, const SimplifyQuery &Q,
                               unsigned MaxRecurse) {
   if (Constant *C = foldOrCommuteConstant(Instruction::Xor, Op0, Op1, Q))
     return C;
 
   // A ^ undef -> undef
-  if (match(Op1, m_Undef()))
+  if (Q.CanUseUndef && match(Op1, m_Undef()))
     return Op1;
 
   // A ^ 0 = A
   if (match(Op1, m_Zero()))
     return Op0;
 
   // A ^ A = 0
   if (Op0 == Op1)
static Value *SimplifyICmpInst(unsigned Predicate, Value *LHS, Value *RHS,
   }
   assert(!isa<UndefValue>(LHS) && "Unexpected icmp undef,%X");
 
   Type *ITy = GetCompareTy(LHS); // The return type.
 
   // For EQ and NE, we can always pick a value for the undef to make the
   // predicate pass or fail, so we can return undef.
   // Matches behavior in llvm::ConstantFoldCompareInstruction.
-  if (isa<UndefValue>(RHS) && ICmpInst::isEquality(Pred))
+  if (Q.CanUseUndef && isa<UndefValue>(RHS) && ICmpInst::isEquality(Pred))
     return UndefValue::get(ITy);
 
   // icmp X, X -> true/false
   // icmp X, undef -> true/false because undef could be X.
-  if (LHS == RHS || isa<UndefValue>(RHS))
+  if (LHS == RHS || (Q.CanUseUndef && isa<UndefValue>(RHS)))
     return ConstantInt::get(ITy, CmpInst::isTrueWhenEqual(Pred));
 
   if (Value *V = simplifyICmpOfBools(Pred, LHS, RHS, Q))
     return V;
 
   if (Value *V = simplifyICmpWithZero(Pred, LHS, RHS, Q))
     return V;
 
static Value *SimplifyFCmpInst(unsigned Predicate, Value *LHS, Value *RHS,
   // NaN is unordered; NaN is not ordered.
   assert((FCmpInst::isOrdered(Pred) || FCmpInst::isUnordered(Pred)) &&
          "Comparison must be either ordered or unordered");
   if (match(RHS, m_NaN()))
     return ConstantInt::get(RetTy, CmpInst::isUnordered(Pred));
 
   // fcmp pred x, undef  and  fcmp pred undef, x
   // fold to true if unordered, false if ordered
-  if (isa<UndefValue>(LHS) || isa<UndefValue>(RHS)) {
+  if (Q.CanUseUndef && (isa<UndefValue>(LHS) || isa<UndefValue>(RHS))) {
     // Choosing NaN for the undef will always make unordered comparison succeed
     // and ordered comparison fail.
     return ConstantInt::get(RetTy, CmpInst::isUnordered(Pred));
   }
 
   // fcmp x,x -> true/false.  Not all compares are foldable.
   if (LHS == RHS) {
     if (CmpInst::isTrueWhenEqual(Pred))

 static Value *SimplifySelectInst(Value *Cond, Value *TrueVal, Value *FalseVal,
                                  const SimplifyQuery &Q, unsigned MaxRecurse) {
   if (auto *CondC = dyn_cast<Constant>(Cond)) {
     if (auto *TrueC = dyn_cast<Constant>(TrueVal))
       if (auto *FalseC = dyn_cast<Constant>(FalseVal))
         return ConstantFoldSelectInstruction(CondC, TrueC, FalseC);
 
     // select undef, X, Y -> X or Y
-    if (isa<UndefValue>(CondC))
+    if (Q.CanUseUndef && isa<UndefValue>(CondC))
       return isa<Constant>(FalseVal) ? FalseVal : TrueVal;
 
     // TODO: Vector constants with undef elements don't simplify.
 
     // select true, X, Y  -> X
     if (CondC->isAllOnesValue())
       return TrueVal;
     // select false, X, Y -> Y
static Value *SimplifySelectInst(Value *Cond, Value *TrueVal, Value *FalseVal,
   if (Cond->getType() == TrueVal->getType() &&
       match(TrueVal, m_One()) && match(FalseVal, m_ZeroInt()))
     return Cond;
 
   // select ?, X, X -> X
   if (TrueVal == FalseVal)
     return TrueVal;
 
-  if (isa<UndefValue>(TrueVal))   // select ?, undef, X -> X
+  if (Q.CanUseUndef && isa<UndefValue>(TrueVal)) // select ?, undef, X -> X
     return FalseVal;
-  if (isa<UndefValue>(FalseVal))   // select ?, X, undef -> X
+  if (Q.CanUseUndef && isa<UndefValue>(FalseVal)) // select ?, X, undef -> X
     return TrueVal;
 
   // Deal with partial undef vector constants: select ?, VecC, VecC' --> VecC''
   Constant *TrueC, *FalseC;
   if (TrueVal->getType()->isVectorTy() && match(TrueVal, m_Constant(TrueC)) &&
       match(FalseVal, m_Constant(FalseC))) {
     unsigned NumElts =
         cast<FixedVectorType>(TrueC->getType())->getNumElements();
     SmallVector<Constant *, 16> NewC;
     for (unsigned i = 0; i != NumElts; ++i) {
       // Bail out on incomplete vector constants.
       Constant *TEltC = TrueC->getAggregateElement(i);
       Constant *FEltC = FalseC->getAggregateElement(i);
       if (!TEltC || !FEltC)
         break;
 
       // If the elements match (undef or not), that value is the result. If only
       // one element is undef, choose the defined element as the safe result.
       if (TEltC == FEltC)
         NewC.push_back(TEltC);
-      else if (isa<UndefValue>(TEltC))
+      else if (Q.CanUseUndef && isa<UndefValue>(TEltC))
         NewC.push_back(FEltC);
-      else if (isa<UndefValue>(FEltC))
+      else if (Q.CanUseUndef && isa<UndefValue>(FEltC))
         NewC.push_back(TEltC);
       else
         break;
     }
     if (NewC.size() == NumElts)
       return ConstantVector::get(NewC);
   }
 
static Value *SimplifyGEPInst(Type *SrcTy, ArrayRef<Value *> Ops,
   // Compute the (pointer) type returned by the GEP instruction.
   Type *LastType = GetElementPtrInst::getIndexedType(SrcTy, Ops.slice(1));
   Type *GEPTy = PointerType::get(LastType, AS);
   if (VectorType *VT = dyn_cast<VectorType>(Ops[0]->getType()))
     GEPTy = VectorType::get(GEPTy, VT->getElementCount());
   else if (VectorType *VT = dyn_cast<VectorType>(Ops[1]->getType()))
     GEPTy = VectorType::get(GEPTy, VT->getElementCount());
 
-  if (isa<UndefValue>(Ops[0]))
+  if (Q.CanUseUndef && isa<UndefValue>(Ops[0]))
     return UndefValue::get(GEPTy);
 
   bool IsScalableVec = isa<ScalableVectorType>(SrcTy);
 
   if (Ops.size() == 2) {
     // getelementptr P, 0 -> P.
     if (match(Ops[1], m_Zero()) && Ops[0]->getType() == GEPTy)
       return Ops[0];

 static Value *SimplifyInsertValueInst(Value *Agg, Value *Val,
                                       ArrayRef<unsigned> Idxs, const SimplifyQuery &Q,
                                       unsigned) {
   if (Constant *CAgg = dyn_cast<Constant>(Agg))
     if (Constant *CVal = dyn_cast<Constant>(Val))
       return ConstantFoldInsertValueInstruction(CAgg, CVal, Idxs);
 
   // insertvalue x, undef, n -> x
-  if (match(Val, m_Undef()))
+  if (Q.CanUseUndef && match(Val, m_Undef()))
     return Agg;
 
   // insertvalue x, (extractvalue y, n), n
   if (ExtractValueInst *EV = dyn_cast<ExtractValueInst>(Val))
     if (EV->getAggregateOperand()->getType() == Agg->getType() &&
         EV->getIndices() == Idxs) {
       // insertvalue undef, (extractvalue y, n), n -> y
-      if (match(Agg, m_Undef()))
+      if (Q.CanUseUndef && match(Agg, m_Undef()))
         return EV->getAggregateOperand();
 
       // insertvalue y, (extractvalue y, n), n -> y
       if (Agg == EV->getAggregateOperand())
         return Agg;
     }
 
   return nullptr;
Value *llvm::SimplifyInsertElementInst(Value *Vec, Value *Val, Value *Idx,
   // For fixed-length vector, fold into undef if index is out of bounds.
   if (auto *CI = dyn_cast<ConstantInt>(Idx)) {
     if (isa<FixedVectorType>(Vec->getType()) &&
         CI->uge(cast<FixedVectorType>(Vec->getType())->getNumElements()))
       return UndefValue::get(Vec->getType());
   }
 
   // If index is undef, it might be out of bounds (see above case)
-  if (isa<UndefValue>(Idx))
+  if (Q.CanUseUndef && isa<UndefValue>(Idx))
     return UndefValue::get(Vec->getType());
 
   // If the scalar is undef, and there is no risk of propagating poison from the
   // vector value, simplify to the vector value.
-  if (isa<UndefValue>(Val) && isGuaranteedNotToBeUndefOrPoison(Vec))
+  if (Q.CanUseUndef && isa<UndefValue>(Val) &&
+      isGuaranteedNotToBeUndefOrPoison(Vec))
     return Vec;
 
   // If we are extracting a value from a vector, then inserting it into the same
   // place, that's the input vector:
   // insertelt Vec, (extractelt Vec, Idx), Idx --> Vec
   if (match(Val, m_ExtractElt(m_Specific(Vec), m_Specific(Idx))))
     return Vec;
 

 
 Value *llvm::SimplifyExtractValueInst(Value *Agg, ArrayRef<unsigned> Idxs,
                                       const SimplifyQuery &Q) {
   return ::SimplifyExtractValueInst(Agg, Idxs, Q, RecursionLimit);
 }
 
 /// Given operands for an ExtractElementInst, see if we can fold the result.
 /// If not, this returns null.
-static Value *SimplifyExtractElementInst(Value *Vec, Value *Idx, const SimplifyQuery &,
-                                         unsigned) {
+static Value *SimplifyExtractElementInst(Value *Vec, Value *Idx,
+                                         const SimplifyQuery &Q, unsigned) {
   auto *VecVTy = cast<VectorType>(Vec->getType());
   if (auto *CVec = dyn_cast<Constant>(Vec)) {
     if (auto *CIdx = dyn_cast<Constant>(Idx))
       return ConstantFoldExtractElementInstruction(CVec, CIdx);
 
     // The index is not relevant if our vector is a splat.
     if (auto *Splat = CVec->getSplatValue())
       return Splat;
 
-    if (isa<UndefValue>(Vec))
+    if (Q.CanUseUndef && isa<UndefValue>(Vec))
       return UndefValue::get(VecVTy->getElementType());
   }
 
   // If extracting a specified index from the vector, see if we can recursively
   // find a previously computed scalar that was inserted into the vector.
   if (auto *IdxC = dyn_cast<ConstantInt>(Idx)) {
     // For fixed-length vector, fold into undef if index is out of bounds.
     if (isa<FixedVectorType>(VecVTy) &&
         IdxC->getValue().uge(cast<FixedVectorType>(VecVTy)->getNumElements()))
       return UndefValue::get(VecVTy->getElementType());
     if (Value *Elt = findScalarElement(Vec, IdxC->getZExtValue()))
       return Elt;
   }
 
   // An undef extract index can be arbitrarily chosen to be an out-of-range
   // index value, which would result in the instruction being undef.
-  if (isa<UndefValue>(Idx))
+  if (Q.CanUseUndef && isa<UndefValue>(Idx))
     return UndefValue::get(VecVTy->getElementType());
 
   return nullptr;
 }
 
 Value *llvm::SimplifyExtractElementInst(Value *Vec, Value *Idx,
                                         const SimplifyQuery &Q) {
   return ::SimplifyExtractElementInst(Vec, Idx, Q, RecursionLimit);
 }
 
 /// See if we can fold the given phi. If not, returns null.
 static Value *SimplifyPHINode(PHINode *PN, const SimplifyQuery &Q) {
   // If all of the PHI's incoming values are the same then replace the PHI node
   // with the common value.
   Value *CommonValue = nullptr;
   bool HasUndefInput = false;
   for (Value *Incoming : PN->incoming_values()) {
     // If the incoming value is the phi node itself, it can safely be skipped.
     if (Incoming == PN) continue;
-    if (isa<UndefValue>(Incoming)) {
+    if (Q.CanUseUndef && isa<UndefValue>(Incoming)) {
       // Remember that we saw an undef value, but otherwise ignore them.
       HasUndefInput = true;
       continue;
     }
     if (CommonValue && Incoming != CommonValue)
       return nullptr;  // Not the same, bail out.
     CommonValue = Incoming;
   }
if (all_of(Indices, [InsertIndex](int MaskElt) {
           VecC[i] = UndefValue::get(C->getType());
       return ConstantVector::get(VecC);
     }
   }
 
   // A shuffle of a splat is always the splat itself. Legal if the shuffle's
   // value type is same as the input vectors' type.
   if (auto *OpShuf = dyn_cast<ShuffleVectorInst>(Op0))
-    if (isa<UndefValue>(Op1) && RetTy == InVecTy &&
+    if (Q.CanUseUndef && isa<UndefValue>(Op1) && RetTy == InVecTy &&
         is_splat(OpShuf->getShuffleMask()))
       return Op0;
 
   // All remaining transformation depend on the value of the mask, which is
   // not known at compile time for scalable vectors.
   if (Scalable)
     return nullptr;
 
case Intrinsic::masked_gather: {
     return nullptr;
   }
   case Intrinsic::fshl:
   case Intrinsic::fshr: {
     Value *Op0 = Call->getArgOperand(0), *Op1 = Call->getArgOperand(1),
           *ShAmtArg = Call->getArgOperand(2);
 
     // If both operands are undef, the result is undef.
-    if (match(Op0, m_Undef()) && match(Op1, m_Undef()))
+    if (Q.CanUseUndef && match(Op0, m_Undef()) && match(Op1, m_Undef()))
       return UndefValue::get(F->getReturnType());
 
     // If shift amount is undef, assume it is zero.
-    if (match(ShAmtArg, m_Undef()))
+    if (Q.CanUseUndef && match(ShAmtArg, m_Undef()))
       return Call->getArgOperand(IID == Intrinsic::fshl ? 0 : 1);
 
     const APInt *ShAmtC;
     if (match(ShAmtArg, m_APInt(ShAmtC))) {
       // If there's effectively no shift, return the 1st arg or 2nd arg.
       APInt BitWidth = APInt(ShAmtC->getBitWidth(), ShAmtC->getBitWidth());
       if (ShAmtC->urem(BitWidth).isNullValue())
         return Call->getArgOperand(IID == Intrinsic::fshl ? 0 : 1);

llvm/lib/Transforms/Scalar/NewGVN.cpp

#endif
   SmallPtrSet<Instruction *, 8> InstructionsToErase;
 
 public:
   NewGVN(Function &F, DominatorTree *DT, AssumptionCache *AC,
          TargetLibraryInfo *TLI, AliasAnalysis *AA, MemorySSA *MSSA,
          const DataLayout &DL)
       : F(F), DT(DT), TLI(TLI), AA(AA), MSSA(MSSA), AC(AC), DL(DL),
         PredInfo(std::make_unique<PredicateInfo>(F, *DT, *AC)),
-        SQ(DL, TLI, DT, AC, /*CtxI=*/nullptr, /*UseInstrInfo=*/false) {}
+        SQ(DL, TLI, DT, AC, /*CtxI=*/nullptr, /*UseInstrInfo=*/false,
+           /*CanUseUndef=*/false) {}
 
   bool runGVN();
 
 private:
   // Expression handling.
   const Expression *createExpression(Instruction *) const;
   const Expression *createBinaryExpression(unsigned, Type *, Value *, Value *,
                                            Instruction *) const;

llvm/test/Transforms/NewGVN/pr33165-distribute-undef.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt -newgvn -S %s | FileCheck %s
 
 ; Test for PR33165.
 
-; TODO: Currently NewGVN miscompiles the function.
 define i2 @f(i2, i1) {
 ; CHECK-LABEL: @f(
 ; CHECK-NEXT:    [[A:%.*]] = xor i2 [[TMP0:%.*]], -1
 ; CHECK-NEXT:    [[B:%.*]] = select i1 [[TMP1:%.*]], i2 [[A]], i2 undef
-; CHECK-NEXT:    ret i2 [[B]]
+; CHECK-NEXT:    [[C:%.*]] = and i2 [[A]], [[B]]
+; CHECK-NEXT:    ret i2 [[C]]
 ;
   %a = xor i2 %0, -1
   %b = select i1 %1, i2 %a, i2 undef
   %c = and i2 %a, %b
   ret i2 %c
 }