[InstrSimplify,NewGVN] Add option to ignore additional instr info when simplifying.

[fhahn]

Summary

NewGVN uses InstructionSimplify for simplifications of leaders of
congruence classes. It is not guaranteed that the metadata or other
flags/keywords (like nsw or exact) of the leader is available for all members
in a congruence class, so we cannot use it for simplification.

This patch adds a InstrInfoQuery struct with a boolean field
UseInstrInfo (which defaults to true to keep the current behavior as
default) and a set of helper methods to get metadata/keywords for a
given instruction, if UseInstrInfo is true. The whole thing might need a
better name, to avoid confusion with TargetInstrInfo but I am not sure
what a better name would be.

The current patch threads through InstrInfoQuery to the required
places, which is messier then it would need to be, if
InstructionSimplify and ValueTracking would share the same Query struct.

The reason I added it as a separate struct is that it can be shared
between InstructionSimplify and ValueTracking's query objects. Also,
some places do not need a full query object, just the InstrInfoQuery.

It also updates some interfaces that do not take a Query object, but a
set of optional parameters to take an additional boolean UseInstrInfo.

See https://bugs.llvm.org/show_bug.cgi?id=37540.

[fhahn]

We could probably do something more clever and propagate the nonnull metadata, if we can ensure that nothing in between can change the nonnullness.

[nikic]

This is going to affect more than only nonnull metadata. For example the same issue can also be reproduced using range metadata:

define i1 @test(i32* %arg, i1 %arg2) {
    br i1 %arg2, label %bb1, label %bb2

bb1:
    %load1 = load i32, i32* %arg
    %cmp1 = icmp eq i32 %load1, 0 
    ret i1 %cmp1

bb2:
    %load2 = load i32, i32* %arg, !range !1
    %cmp2 = icmp eq i32 %load2, 0 
    ret i1 %cmp2
}

!1 = !{ i32 1, i32 10 }

[• dberlin]

Hey Florian,
Thanks for looking at this:
IMHO, We should be storing and hashing/comparing the relevant info in the loadexpression (IE store a bit that it's nonnull)
I really do not want to end up with complex business logic in the equals function, that's a thing the callers should be worrying about.

[• dberlin]

Yeah, some of this metadata travels backwards in time (IE if you can prove they are equivalent, the metadata must apply) and some of it is not.

This is the moral equivalent of some of the GVN bugs we also have where we use things we discover later to prove earlier things.

[fhahn]

In D47143#1106573, @dberlin wrote:

Hey Florian,
Thanks for looking at this:
IMHO, We should be storing and hashing/comparing the relevant info in the loadexpression (IE store a bit that it's nonnull)
I really do not want to end up with complex business logic in the equals function, that's a thing the callers should be worrying about.

Will do thanks. As @nikic mentioned we have to account for more than nonnull, which I'll do once nonnull is dealt with properly.

[• dberlin]

So, actually, this *is* a variant the same issue we have elsewhere with Simplify* (and there are a few newgvn bugs open about it). These actually *are* congruent, we just don't want it simplifying this hard.

I don't think you want to say these are non-congruent, because you can in fact replace load2 with load1 and load1 with load2, we would just merge metadata.
I think we want to be able to tell it what information it's allowed to use when simplifying.

I believe NewGVN is always going to be able to discover things that will be confusing to the simplifier as long as the simplifier is doing super-complex IR walking and understanding.

[fhahn]

In D47143#1106580, @dberlin wrote:

So, actually, this *is* a variant the same issue we have elsewhere with Simplify* (and there are a few newgvn bugs open about it). These actually *are* congruent, we just don't want it simplifying this hard.

I don't think you want to say these are non-congruent, because you can in fact replace load2 with load1 and load1 with load2, we would just merge metadata.
I think we want to be able to tell it what information it's allowed to use when simplifying.

Right, that was what I thought initially as well. We should be better off if we manage to limit simplifications. Unfortunately, there seems to be no option to ignore certain metadata by Simplify*. I suppose we could drop the metadata before we pass instructions to Simplify or maintain a set merged metadata for the congruence classes and use that when passing an instruction to Simplify*. The latter would still allow us to use the nonnull metadata for example, if all members have it, but it may not be worth maintaining a set of merged metadata...

Or do you think it would be better to teach Simplify* to ignore (some) metadata? This seems like a non-trivial change.

What do you think?

[• dberlin]

One of the reasons i moved Simplify* to take a SimplifyQuery is so that things like this are possible to add without a tremendous amount of work in Simplify*.

Part of me thinks anything else would be too fragile, but i'm open to ideas.
We are basically using it as a more advanced constant folder that does some reassociation and simplification.

[nikic]

@fhahn It's probably not feasible to remove metadata or explicitly pass merged metadata as the relevant metadata may be somewhere further up the chain. E.g. we might not be looking at ICmp(Load, null) but at something like ICmp(BitCast(GEP(Load)), null), which will probably be subject to the same issues, as these checks are recursive.

Having the option to ignore metadata (or maybe have a lambda that determines whether metadata can be used, e.g. based on common metadata in all members of the congruence, or because of a dominating leader) sounds like a more robust solution.

[• dberlin]

Right, this is why we haven't done anything yet past taht - i ran out of time to start driving through a real plan :)

[nikic]

For reference, another bug relating to NewGVN + Simplify is https://bugs.llvm.org/show_bug.cgi?id=34093. In that case it's not a matter of metadata, but an equivalence that was established based on predicateinfo is assumed to hold prior to the corresponding ssa.copy, resulting in incorrect simplification.

https://bugs.llvm.org/show_bug.cgi?id=31792 is not a miscompile but rather a missed optimization. GVN knows that two GEPs reference the same base, but simplification doesn't.

I'm finding it hard to put these three issues under one umbrella. They all need some kind of additional interaction with simplification to be resolved, but the specifics seem to be quite different in each case, it's not just a matter of a single flag.

[• dberlin]

Sure, you will never find a single flag to control all of this, my goal was to be able to have flags at all to control the various issues.

(the alternative is to move back to ConstantFold* instead of Simplify*, which would require reimplementing our own simplification and reassociation stuff that we care about.)

[fhahn]

Thanks for the excellent feedback. I'll look into teaching Simplify* to skip (some) metadata

[• dberlin]

SGTM.
If it turns out to be way too ugly. let's circle back and figure out what
we want to do.

[fhahn]

Updated to ignore metadata while simplifying with InstructionSimplify. I've updated the description with details.

It adds a very crude way to disable the use of metadata. Not sure if we want a more fine grained approach. The cosmetics of the UseMetadata handling can be improved slightly, happy to do that if the overall approach is sensible.

[fhahn]

ping

[fhahn]

ping. This introduces a way to prevent InstructionSimplify from using metadata when simplifying for NewGVN. This fixes a correctness problem, where we currently make a simplification based on metadata of the leader, which not necessarily is available for all instructions in a congruence class. Any feedback on the general approach would be great.

[efriedma]

Is this supposed to apply specifically to metadata, or just anything that NewGVN doesn't consider while numbering? Some InstSimplify transforms check, for example, nsw.

You need to make sure you don't call into other places that might query metadata. It looks like you caught isKnownNonEqual/isKnownNonZero, but there's also computeKnownBits and isKnownToBeAPowerOfTwo.

[fhahn]

In D47143#1174501, @efriedma wrote:

Is this supposed to apply specifically to metadata, or just anything that NewGVN doesn't consider while numbering? Some InstSimplify transforms check, for example, nsw.

I think we cannot use anything for simplification that's not available for all members of a congruence class. Thanks for pointing me to nsw, I'll have a look and see how to best generalize this.

You need to make sure you don't call into other places that might query metadata. It looks like you caught isKnownNonEqual/isKnownNonZero, but there's also computeKnownBits and isKnownToBeAPowerOfTwo.

Ah yes thanks! Unfortunately there's quite a big set of functions to cover. If this kind of interface makes sense, I'll take another close look.

[fhahn]

I've updated the patch to have a query object which provides convenience wrappers around getMetadata, hasNo* and isExact, which return conservative results if it's not safe to use any of those. I've also threaded this through at a lot more places and added a few test cases with nsw/exact.

I also updated the title and the description with more details. What do you think of the overall approach? There is some refactoring which might make this slightly less invasive, as using the function versions that take a Query object everywhere internally in ValueTracking.cpp/InstructionSimplify.cpp

[fhahn]

ping

[hiraditya]

LGTM with the minor nit.

include/llvm/Analysis/InstructionSimplify.h

84

I see other places use isa to first check if BinaryOperator isa PossiblyExactOperator , or a dyn_cast maybe.

[fhahn]

In D47143#1199218, @hiraditya wrote:

LGTM with the minor nit.

Thanks for having a look!

I went through the list of outstanding NewGVN issues today and the issue fixed in this patch was causing 3-4 reported miscompiles with NewGVN. Unfortunately there are some cases where we could still use metadata while simplifying (e.g. if all members of the congruence class have the same metadata), but I don't see a sane way of ensuring that (e.g. new members without metadata could be added to a class after we simplified the leader).

include/llvm/Analysis/InstructionSimplify.h

84

Will update before committing, thanks!

[hiraditya]

Accepting because there are no alternate suggestions, and this patch fixes miscompiles preventing NewGVN to be used in some cases.

include/llvm/Analysis/InstructionSimplify.h

 
 struct SimplifyQuery {
   const DataLayout &DL;
   const TargetLibraryInfo *TLI = nullptr;
   const DominatorTree *DT = nullptr;
   AssumptionCache *AC = nullptr;
   const Instruction *CxtI = nullptr;
 
+  /// If true, it is safe to use metadata during simplification.
+  bool UseMetadata = true;
+
   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)
-      : DL(DL), TLI(TLI), DT(DT), AC(AC), CxtI(CXTI) {}
+                const Instruction *CXTI = nullptr, bool UseMetadata = true)
+      : DL(DL), TLI(TLI), DT(DT), AC(AC), CxtI(CXTI), UseMetadata(UseMetadata) {
+  }
   SimplifyQuery getWithInstruction(Instruction *I) const {
     SimplifyQuery Copy(*this);
     Copy.CxtI = I;
     return Copy;
   }
 };
 
 // NOTE: the explicit multiple argument versions of these functions are
 // deprecated.
 // Please use the SimplifyQuery versions in new code.
 
 /// Given operands for an Add, fold the result or return null.
 Value *SimplifyAddInst(Value *LHS, Value *RHS, bool isNSW, bool isNUW,
                        const SimplifyQuery &Q);
 
 /// Given operands for a Sub, fold the result or return null.
 Value *SimplifySubInst(Value *LHS, Value *RHS, bool isNSW, bool isNUW,
                        const SimplifyQuery &Q);
 
 /// Given operands for an FAdd, fold the result or return null.

include/llvm/Analysis/ValueTracking.h

class Value;
   /// vectors, return true if every element is known to be non-zero when
   /// defined. For pointers, if the context instruction and dominator tree are
   /// specified, perform context-sensitive analysis and return true if the
   /// pointer couldn't possibly be null at the specified instruction.
   /// Supports values with integer or pointer type and vectors of integers.
   bool isKnownNonZero(const Value *V, const DataLayout &DL, unsigned Depth = 0,
                       AssumptionCache *AC = nullptr,
                       const Instruction *CxtI = nullptr,
-                      const DominatorTree *DT = nullptr);
+                      const DominatorTree *DT = nullptr,
+                      bool UseMetadata = true);
 
   /// Returns true if the give value is known to be non-negative.
   bool isKnownNonNegative(const Value *V, const DataLayout &DL,
                           unsigned Depth = 0,
                           AssumptionCache *AC = nullptr,
                           const Instruction *CxtI = nullptr,
                           const DominatorTree *DT = nullptr);
 
class Value;
   bool isKnownNegative(const Value *V, const DataLayout &DL, unsigned Depth = 0,
                        AssumptionCache *AC = nullptr,
                        const Instruction *CxtI = nullptr,
                        const DominatorTree *DT = nullptr);
 
   /// Return true if the given values are known to be non-equal when defined.
   /// Supports scalar integer types only.
   bool isKnownNonEqual(const Value *V1, const Value *V2, const DataLayout &DL,
                        AssumptionCache *AC = nullptr,
                        const Instruction *CxtI = nullptr,
-                      const DominatorTree *DT = nullptr);
+                       const DominatorTree *DT = nullptr,
+                       bool UseMetadata = true);
 
   /// Return true if 'V & Mask' is known to be zero. We use this predicate to
   /// simplify operations downstream. Mask is known to be zero for bits that V
   /// cannot have.
   ///
   /// This function is defined on values with integer type, values with pointer
   /// type, and vectors of integers.  In the case
   /// where V is a vector, the mask, known zero, and known one values are the

lib/Analysis/InstructionSimplify.cpp

 //
 // If the C and C++ standards are ever made sufficiently restrictive in this
 // area, it may be possible to update LLVM's semantics accordingly and reinstate
 // this optimization.
 static Constant *
 computePointerICmp(const DataLayout &DL, const TargetLibraryInfo *TLI,
                    const DominatorTree *DT, CmpInst::Predicate Pred,
                    AssumptionCache *AC, const Instruction *CxtI,
-                   Value *LHS, Value *RHS) {
+                   bool UseMetadata, Value *LHS, Value *RHS) {
   // First, skip past any trivial no-ops.
   LHS = LHS->stripPointerCasts();
   RHS = RHS->stripPointerCasts();
 
   // A non-null pointer is not equal to a null pointer.
-  if (llvm::isKnownNonZero(LHS, DL) && isa<ConstantPointerNull>(RHS) &&
+  if (llvm::isKnownNonZero(LHS, DL, 0, nullptr, nullptr, nullptr,
+                           UseMetadata) &&
+      isa<ConstantPointerNull>(RHS) &&
       (Pred == CmpInst::ICMP_EQ || Pred == CmpInst::ICMP_NE))
     return ConstantInt::get(GetCompareTy(LHS),
                             !CmpInst::isTrueWhenEqual(Pred));
 
   // We can only fold certain predicates on pointer comparisons.
   switch (Pred) {
   default:
     return nullptr;
static Value *simplifyICmpWithZero(CmpInst::Predicate Pred, Value *LHS,
   default:
     llvm_unreachable("Unknown ICmp predicate!");
   case ICmpInst::ICMP_ULT:
     return getFalse(ITy);
   case ICmpInst::ICMP_UGE:
     return getTrue(ITy);
   case ICmpInst::ICMP_EQ:
   case ICmpInst::ICMP_ULE:
-    if (isKnownNonZero(LHS, Q.DL, 0, Q.AC, Q.CxtI, Q.DT))
+    if (isKnownNonZero(LHS, Q.DL, 0, Q.AC, Q.CxtI, Q.DT, Q.UseMetadata))
       return getFalse(ITy);
     break;
   case ICmpInst::ICMP_NE:
   case ICmpInst::ICMP_UGT:
-    if (isKnownNonZero(LHS, Q.DL, 0, Q.AC, Q.CxtI, Q.DT))
+    if (isKnownNonZero(LHS, Q.DL, 0, Q.AC, Q.CxtI, Q.DT, Q.UseMetadata))
       return getTrue(ITy);
     break;
   case ICmpInst::ICMP_SLT: {
     KnownBits LHSKnown = computeKnownBits(LHS, Q.DL, 0, Q.AC, Q.CxtI, Q.DT);
     if (LHSKnown.isNegative())
       return getTrue(ITy);
     if (LHSKnown.isNonNegative())
       return getFalse(ITy);
case Instruction::URem:
     break;
 
   default:
     break;
   }
 }
 
 static Value *simplifyICmpWithConstant(CmpInst::Predicate Pred, Value *LHS,
-                                       Value *RHS) {
+                                       Value *RHS, bool UseMetadata) {
   Type *ITy = GetCompareTy(RHS); // The return type.
 
   Value *X;
   // Sign-bit checks can be optimized to true/false after unsigned
   // floating-point casts:
   // icmp slt (bitcast (uitofp X)),  0 --> false
   // icmp sgt (bitcast (uitofp X)), -1 --> true
   if (match(LHS, m_BitCast(m_UIToFP(m_Value(X))))) {
static Value *simplifyICmpWithConstant(CmpInst::Predicate Pred, Value *LHS,
   APInt Lower = APInt(Width, 0);
   APInt Upper = APInt(Width, 0);
   if (auto *BO = dyn_cast<BinaryOperator>(LHS))
     setLimitsForBinOp(*BO, Lower, Upper);
 
   ConstantRange LHS_CR =
       Lower != Upper ? ConstantRange(Lower, Upper) : ConstantRange(Width, true);
 
+  if (UseMetadata)
     if (auto *I = dyn_cast<Instruction>(LHS))
       if (auto *Ranges = I->getMetadata(LLVMContext::MD_range))
         LHS_CR = LHS_CR.intersectWith(getConstantRangeFromMetadata(*Ranges));
 
   if (!LHS_CR.isFullSet()) {
     if (RHS_CR.contains(LHS_CR))
       return ConstantInt::getTrue(ITy);
     if (RHS_CR.inverse().contains(LHS_CR))
       return ConstantInt::getFalse(ITy);
   }
 
if (LHS == RHS || 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;
 
-  if (Value *V = simplifyICmpWithConstant(Pred, LHS, RHS))
+  if (Value *V = simplifyICmpWithConstant(Pred, LHS, RHS, Q.UseMetadata))
     return V;
 
   // If both operands have range metadata, use the metadata
   // to simplify the comparison.
   if (isa<Instruction>(RHS) && isa<Instruction>(LHS)) {
     auto RHS_Instr = cast<Instruction>(RHS);
     auto LHS_Instr = cast<Instruction>(LHS);
 
-    if (RHS_Instr->getMetadata(LLVMContext::MD_range) &&
+    if (Q.UseMetadata && RHS_Instr->getMetadata(LLVMContext::MD_range) &&
         LHS_Instr->getMetadata(LLVMContext::MD_range)) {
       auto RHS_CR = getConstantRangeFromMetadata(
           *RHS_Instr->getMetadata(LLVMContext::MD_range));
       auto LHS_CR = getConstantRangeFromMetadata(
           *LHS_Instr->getMetadata(LLVMContext::MD_range));
 
       auto Satisfied_CR = ConstantRange::makeSatisfyingICmpRegion(Pred, RHS_CR);
       if (Satisfied_CR.contains(LHS_CR))
if (isa<SExtInst>(LHS)) {
           }
         }
       }
     }
   }
 
   // icmp eq|ne X, Y -> false|true if X != Y
   if (ICmpInst::isEquality(Pred) &&
-      isKnownNonEqual(LHS, RHS, Q.DL, Q.AC, Q.CxtI, Q.DT)) {
+      isKnownNonEqual(LHS, RHS, Q.DL, Q.AC, Q.CxtI, Q.DT, Q.UseMetadata)) {
     return Pred == ICmpInst::ICMP_NE ? getTrue(ITy) : getFalse(ITy);
   }
 
   if (Value *V = simplifyICmpWithBinOp(Pred, LHS, RHS, Q, MaxRecurse))
     return V;
 
   if (Value *V = simplifyICmpWithMinMax(Pred, LHS, RHS, Q, MaxRecurse))
     return V;
 
   // Simplify comparisons of related pointers using a powerful, recursive
   // GEP-walk when we have target data available..
   if (LHS->getType()->isPointerTy())
-    if (auto *C = computePointerICmp(Q.DL, Q.TLI, Q.DT, Pred, Q.AC, Q.CxtI, LHS,
-                                     RHS))
+    if (auto *C = computePointerICmp(Q.DL, Q.TLI, Q.DT, Pred, Q.AC, Q.CxtI,
+                                     Q.UseMetadata, LHS, RHS))
       return C;
   if (auto *CLHS = dyn_cast<PtrToIntOperator>(LHS))
     if (auto *CRHS = dyn_cast<PtrToIntOperator>(RHS))
       if (Q.DL.getTypeSizeInBits(CLHS->getPointerOperandType()) ==
               Q.DL.getTypeSizeInBits(CLHS->getType()) &&
           Q.DL.getTypeSizeInBits(CRHS->getPointerOperandType()) ==
               Q.DL.getTypeSizeInBits(CRHS->getType()))
-        if (auto *C = computePointerICmp(Q.DL, Q.TLI, Q.DT, Pred, Q.AC, Q.CxtI,
-                                         CLHS->getPointerOperand(),
-                                         CRHS->getPointerOperand()))
+        if (auto *C = computePointerICmp(
+                Q.DL, Q.TLI, Q.DT, Pred, Q.AC, Q.CxtI, Q.UseMetadata,
+                CLHS->getPointerOperand(), CRHS->getPointerOperand()))
           return C;
 
   if (GetElementPtrInst *GLHS = dyn_cast<GetElementPtrInst>(LHS)) {
     if (GEPOperator *GRHS = dyn_cast<GEPOperator>(RHS)) {
       if (GLHS->getPointerOperand() == GRHS->getPointerOperand() &&
           GLHS->hasAllConstantIndices() && GRHS->hasAllConstantIndices() &&
           (ICmpInst::isEquality(Pred) ||
            (GLHS->isInBounds() && GRHS->isInBounds() &&

lib/Analysis/ValueTracking.cpp

struct Query {
   /// computeKnownBits to repeatedly visit the same assume intrinsic. The
   /// classic case of this is assume(x = y), which will attempt to determine
   /// bits in x from bits in y, which will attempt to determine bits in y from
   /// bits in x, etc. Regarding the mutual recursion, computeKnownBits can call
   /// isKnownNonZero, which calls computeKnownBits and isKnownToBeAPowerOfTwo
   /// (all of which can call computeKnownBits), and so on.
   std::array<const Value *, MaxDepth> Excluded;
 
+  /// If true, it is safe to use metadata during simplification.
+  bool UseMetadata;
+
   unsigned NumExcluded = 0;
 
   Query(const DataLayout &DL, AssumptionCache *AC, const Instruction *CxtI,
-        const DominatorTree *DT, OptimizationRemarkEmitter *ORE = nullptr)
-      : DL(DL), AC(AC), CxtI(CxtI), DT(DT), ORE(ORE) {}
+        const DominatorTree *DT, OptimizationRemarkEmitter *ORE = nullptr,
+        bool UseMetadata = true)
+      : DL(DL), AC(AC), CxtI(CxtI), DT(DT), ORE(ORE), UseMetadata(UseMetadata) {
+  }
 
   Query(const Query &Q, const Value *NewExcl)
       : DL(Q.DL), AC(Q.AC), CxtI(Q.CxtI), DT(Q.DT), ORE(Q.ORE),
-        NumExcluded(Q.NumExcluded) {
+        UseMetadata(Q.UseMetadata), NumExcluded(Q.NumExcluded) {
     Excluded = Q.Excluded;
     Excluded[NumExcluded++] = NewExcl;
     assert(NumExcluded <= Excluded.size());
   }
 
   bool isExcluded(const Value *Value) const {
     if (NumExcluded == 0)
       return false;
bool llvm::isKnownToBeAPowerOfTwo(const Value *V, const DataLayout &DL,
   return ::isKnownToBeAPowerOfTwo(V, OrZero, Depth,
                                   Query(DL, AC, safeCxtI(V, CxtI), DT));
 }
 
 static bool isKnownNonZero(const Value *V, unsigned Depth, const Query &Q);
 
 bool llvm::isKnownNonZero(const Value *V, const DataLayout &DL, unsigned Depth,
                           AssumptionCache *AC, const Instruction *CxtI,
-                          const DominatorTree *DT) {
-  return ::isKnownNonZero(V, Depth, Query(DL, AC, safeCxtI(V, CxtI), DT));
+                          const DominatorTree *DT, bool UseMetadata) {
+  return ::isKnownNonZero(
+      V, Depth, Query(DL, AC, safeCxtI(V, CxtI), DT, nullptr, UseMetadata));
 }
 
 bool llvm::isKnownNonNegative(const Value *V, const DataLayout &DL,
                               unsigned Depth,
                               AssumptionCache *AC, const Instruction *CxtI,
                               const DominatorTree *DT) {
   KnownBits Known = computeKnownBits(V, DL, Depth, AC, CxtI, DT);
   return Known.isNonNegative();
bool llvm::isKnownNegative(const Value *V, const DataLayout &DL, unsigned Depth,
                            const DominatorTree *DT) {
   KnownBits Known = computeKnownBits(V, DL, Depth, AC, CxtI, DT);
   return Known.isNegative();
 }
 
 static bool isKnownNonEqual(const Value *V1, const Value *V2, const Query &Q);
 
 bool llvm::isKnownNonEqual(const Value *V1, const Value *V2,
-                           const DataLayout &DL,
-                           AssumptionCache *AC, const Instruction *CxtI,
-                           const DominatorTree *DT) {
-  return ::isKnownNonEqual(V1, V2, Query(DL, AC,
-                                         safeCxtI(V1, safeCxtI(V2, CxtI)),
-                                         DT));
+                           const DataLayout &DL, AssumptionCache *AC,
+                           const Instruction *CxtI, const DominatorTree *DT,
+                           bool UseMetadata) {
+  return ::isKnownNonEqual(V1, V2,
+                           Query(DL, AC, safeCxtI(V1, safeCxtI(V2, CxtI)), DT,
+                                 /*ORE=*/nullptr, /*UseMetadata=*/UseMetadata));
 }
 
 static bool MaskedValueIsZero(const Value *V, const APInt &Mask, unsigned Depth,
                               const Query &Q);
 
 bool llvm::MaskedValueIsZero(const Value *V, const APInt &Mask,
                              const DataLayout &DL,
                              unsigned Depth, AssumptionCache *AC,

 static void computeKnownBitsFromOperator(const Operator *I, KnownBits &Known,
                                          unsigned Depth, const Query &Q) {
   unsigned BitWidth = Known.getBitWidth();
 
   KnownBits Known2(Known);
   switch (I->getOpcode()) {
   default: break;
   case Instruction::Load:
+    if (Q.UseMetadata)
       if (MDNode *MD = cast<LoadInst>(I)->getMetadata(LLVMContext::MD_range))
         computeKnownBitsFromRangeMetadata(*MD, Known);
     break;
   case Instruction::And: {
     // If either the LHS or the RHS are Zero, the result is zero.
     computeKnownBits(I->getOperand(1), Known, Depth + 1, Q);
     computeKnownBits(I->getOperand(0), Known2, Depth + 1, Q);
 
     // Output known-1 bits are only known if set in both the LHS & RHS.
     Known.One &= Known2.One;
case Instruction::PHI: {
     }
     break;
   }
   case Instruction::Call:
   case Instruction::Invoke:
     // If range metadata is attached to this call, set known bits from that,
     // and then intersect with known bits based on other properties of the
     // function.
+    if (Q.UseMetadata)
       if (MDNode *MD = cast<Instruction>(I)->getMetadata(LLVMContext::MD_range))
         computeKnownBitsFromRangeMetadata(*MD, Known);
     if (const Value *RV = ImmutableCallSite(I).getReturnedArgOperand()) {
       computeKnownBits(RV, Known2, Depth + 1, Q);
       Known.Zero |= Known2.Zero;
       Known.One |= Known2.One;
     }
     if (const IntrinsicInst *II = dyn_cast<IntrinsicInst>(I)) {
       switch (II->getIntrinsicID()) {
       default: break;
if (auto *C = dyn_cast<Constant>(V)) {
     if (const GlobalValue *GV = dyn_cast<GlobalValue>(V)) {
       if (!GV->isAbsoluteSymbolRef() && !GV->hasExternalWeakLinkage() &&
           GV->getType()->getAddressSpace() == 0)
         return true;
     } else
       return false;
   }
 
+  if (Q.UseMetadata) {
     if (auto *I = dyn_cast<Instruction>(V)) {
       if (MDNode *Ranges = I->getMetadata(LLVMContext::MD_range)) {
         // If the possible ranges don't contain zero, then the value is
         // definitely non-zero.
         if (auto *Ty = dyn_cast<IntegerType>(V->getType())) {
           const APInt ZeroValue(Ty->getBitWidth(), 0);
           if (rangeMetadataExcludesValue(Ranges, ZeroValue))
             return true;
         }
       }
     }
+  }
 
   // Some of the tests below are recursive, so bail out if we hit the limit.
   if (Depth++ >= MaxDepth)
     return false;
 
   // Check for pointer simplifications.
   if (V->getType()->isPointerTy()) {
     // Alloca never returns null, malloc might.
     if (isa<AllocaInst>(V) && Q.DL.getAllocaAddrSpace() == 0)
       return true;
 
+    if (Q.UseMetadata) {
       // A byval, inalloca, or nonnull argument is never null.
       if (const Argument *A = dyn_cast<Argument>(V))
         if (A->hasByValOrInAllocaAttr() || A->hasNonNullAttr())
           return true;
 
       // A Load tagged with nonnull metadata is never null.
       if (const LoadInst *LI = dyn_cast<LoadInst>(V))
         if (LI->getMetadata(LLVMContext::MD_nonnull))
           return true;
+    }
 
     if (auto CS = ImmutableCallSite(V)) {
       if (CS.isReturnNonNull())
         return true;
       if (const auto *RP = getArgumentAliasingToReturnedPointer(CS))
         return isKnownNonZero(RP, Depth, Q);
     }
   }

lib/Transforms/Scalar/NewGVN.cpp

#endif
   // Deletion info.
   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), DL(DL),
-        PredInfo(make_unique<PredicateInfo>(F, *DT, *AC)), SQ(DL, TLI, DT, AC) {
-  }
+        PredInfo(make_unique<PredicateInfo>(F, *DT, *AC)),
+        SQ(DL, TLI, DT, AC, /*CtxI=*/nullptr, /*UseMetadata=*/false) {}
 
   bool runGVN();
 
 private:
   // Expression handling.
   const Expression *createExpression(Instruction *) const;
   const Expression *createBinaryExpression(unsigned, Type *, Value *, Value *,
                                            Instruction *) const;

test/Transforms/NewGVN/metadata-simplify.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+
+; The tests in this file check that we do not simplify based on metadata that is
+; not available on all code paths.
+
+; RUN: opt < %s -S -newgvn | FileCheck %s
+
+define i1 @test1(i32** %arg, i1 %arg2) {
+; CHECK-LABEL: @test1(
+; CHECK-NEXT:    br i1 [[ARG2:%.*]], label [[BB1:%.*]], label [[BB2:%.*]]
+; CHECK:       bb1:
+; CHECK-NEXT:    [[LOAD1:%.*]] = load i32*, i32** [[ARG:%.*]], !nonnull !0
+; CHECK-NEXT:    [[CMP1:%.*]] = icmp eq i32* [[LOAD1]], null
+; CHECK-NEXT:    ret i1 [[CMP1]]
+; CHECK:       bb2:
+; CHECK-NEXT:    [[LOAD2:%.*]] = load i32*, i32** [[ARG]]
+; CHECK-NEXT:    [[CMP2:%.*]] = icmp eq i32* [[LOAD2]], null
+; CHECK-NEXT:    ret i1 [[CMP2]]
+;
+  br i1 %arg2, label %bb1, label %bb2
+
+bb1:
+  %load1 = load i32*, i32** %arg, !nonnull !0
+  %cmp1 = icmp eq i32* %load1, null
+  ret i1 %cmp1
+
+bb2:
+  %load2 = load i32*, i32** %arg
+  %cmp2 = icmp eq i32* %load2, null
+  ret i1 %cmp2
+}
+
+define i1 @test2(i32** %arg, i1 %arg2) {
+; CHECK-LABEL: @test2(
+; CHECK-NEXT:    br i1 [[ARG2:%.*]], label [[BB1:%.*]], label [[BB2:%.*]]
+; CHECK:       bb1:
+; CHECK-NEXT:    [[LOAD1:%.*]] = load i32*, i32** [[ARG:%.*]]
+; CHECK-NEXT:    [[CMP1:%.*]] = icmp eq i32* [[LOAD1]], null
+; CHECK-NEXT:    ret i1 [[CMP1]]
+; CHECK:       bb2:
+; CHECK-NEXT:    [[LOAD2:%.*]] = load i32*, i32** [[ARG]], !nonnull !0
+; CHECK-NEXT:    [[CMP2:%.*]] = icmp eq i32* [[LOAD2]], null
+; CHECK-NEXT:    ret i1 [[CMP2]]
+;
+  br i1 %arg2, label %bb1, label %bb2
+
+bb1:
+  %load1 = load i32*, i32** %arg
+  %cmp1 = icmp eq i32* %load1, null
+  ret i1 %cmp1
+
+bb2:
+  %load2 = load i32*, i32** %arg, !nonnull !0
+  %cmp2 = icmp eq i32* %load2, null
+  ret i1 %cmp2
+}
+
+
+define i1 @test3(i32* %ptr, i1 %arg2) {
+; CHECK-LABEL: @test3(
+; CHECK-NEXT:    br i1 [[ARG2:%.*]], label [[BB1:%.*]], label [[BB2:%.*]]
+; CHECK:       bb1:
+; CHECK-NEXT:    [[LOAD1:%.*]] = load i32, i32* [[PTR:%.*]], !range !1
+; CHECK-NEXT:    [[CMP1:%.*]] = icmp ne i32 [[LOAD1]], 999
+; CHECK-NEXT:    ret i1 [[CMP1]]
+; CHECK:       bb2:
+; CHECK-NEXT:    [[LOAD2:%.*]] = load i32, i32* [[PTR]]
+; CHECK-NEXT:    [[CMP2:%.*]] = icmp ne i32 [[LOAD2]], 999
+; CHECK-NEXT:    ret i1 [[CMP2]]
+;
+  br i1 %arg2, label %bb1, label %bb2
+
+bb1:
+  %load1 = load i32, i32* %ptr, !range !1
+  %cmp1 = icmp ne i32 %load1, 999
+  ret i1 %cmp1
+
+bb2:
+  %load2 = load i32, i32* %ptr
+  %cmp2 = icmp ne i32 %load2, 999
+  ret i1 %cmp2
+}
+
+define i1 @test4(i32* %ptr, i1 %arg2) {
+; CHECK-LABEL: @test4(
+; CHECK-NEXT:    br i1 [[ARG2:%.*]], label [[BB1:%.*]], label [[BB2:%.*]]
+; CHECK:       bb1:
+; CHECK-NEXT:    [[LOAD1:%.*]] = load i32, i32* [[PTR:%.*]]
+; CHECK-NEXT:    [[CMP1:%.*]] = icmp ne i32 [[LOAD1]], 999
+; CHECK-NEXT:    ret i1 [[CMP1]]
+; CHECK:       bb2:
+; CHECK-NEXT:    [[LOAD2:%.*]] = load i32, i32* [[PTR]], !range !1
+; CHECK-NEXT:    [[CMP2:%.*]] = icmp ne i32 [[LOAD2]], 999
+; CHECK-NEXT:    ret i1 [[CMP2]]
+;
+  br i1 %arg2, label %bb1, label %bb2
+
+bb1:
+  %load1 = load i32, i32* %ptr
+  %cmp1 = icmp ne i32 %load1, 999
+  ret i1 %cmp1
+
+bb2:
+  %load2 = load i32, i32* %ptr, !range !1
+  %cmp2 = icmp ne i32 %load2, 999
+  ret i1 %cmp2
+}
+
+define i1 @test5(i32* %ptr, i1 %arg2) {
+; CHECK-LABEL: @test5(
+; CHECK-NEXT:    br i1 [[ARG2:%.*]], label [[BB1:%.*]], label [[BB2:%.*]]
+; CHECK:       bb1:
+; CHECK-NEXT:    [[LOAD1:%.*]] = load i32, i32* [[PTR:%.*]], !range !1
+; CHECK-NEXT:    [[CMP1:%.*]] = icmp slt i32 [[LOAD1]], 999
+; CHECK-NEXT:    ret i1 [[CMP1]]
+; CHECK:       bb2:
+; CHECK-NEXT:    [[LOAD2:%.*]] = load i32, i32* [[PTR]]
+; CHECK-NEXT:    [[CMP2:%.*]] = icmp slt i32 [[LOAD2]], 999
+; CHECK-NEXT:    ret i1 [[CMP2]]
+;
+  br i1 %arg2, label %bb1, label %bb2
+
+bb1:
+  %load1 = load i32, i32* %ptr, !range !1
+  %cmp1 = icmp slt i32 %load1, 999
+  ret i1 %cmp1
+
+bb2:
+  %load2 = load i32, i32* %ptr
+  %cmp2 = icmp slt i32 %load2, 999
+  ret i1 %cmp2
+}
+
+define i1 @test6(i32* %ptr, i1 %arg2) {
+; CHECK-LABEL: @test6(
+; CHECK-NEXT:    br i1 [[ARG2:%.*]], label [[BB1:%.*]], label [[BB2:%.*]]
+; CHECK:       bb1:
+; CHECK-NEXT:    [[LOAD1:%.*]] = load i32, i32* [[PTR:%.*]]
+; CHECK-NEXT:    [[CMP1:%.*]] = icmp slt i32 [[LOAD1]], 999
+; CHECK-NEXT:    ret i1 [[CMP1]]
+; CHECK:       bb2:
+; CHECK-NEXT:    [[LOAD2:%.*]] = load i32, i32* [[PTR]], !range !1
+; CHECK-NEXT:    [[CMP2:%.*]] = icmp slt i32 [[LOAD2]], 999
+; CHECK-NEXT:    ret i1 [[CMP2]]
+;
+  br i1 %arg2, label %bb1, label %bb2
+
+bb1:
+  %load1 = load i32, i32* %ptr
+  %cmp1 = icmp slt i32 %load1, 999
+  ret i1 %cmp1
+
+bb2:
+  %load2 = load i32, i32* %ptr, !range !1
+  %cmp2 = icmp slt i32 %load2, 999
+  ret i1 %cmp2
+}
+
+!0 = !{}
+!1 = !{ i32 10, i32 20 }