From 160767120931b1e3df748c6d982583a4b890e83c Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Fri, 7 Feb 2025 15:11:34 +0000 Subject: [PATCH 1/2] InstSimplify: improve computePointerICmp (NFC) The comment about inbounds protecting only against unsigned wrapping is incorrect: it also protects against signed wrapping, but the issue is that it could cross the sign boundary. --- llvm/lib/Analysis/InstructionSimplify.cpp | 28 +++++++---------------- 1 file changed, 8 insertions(+), 20 deletions(-) diff --git a/llvm/lib/Analysis/InstructionSimplify.cpp b/llvm/lib/Analysis/InstructionSimplify.cpp index 3cbc4107433ef..2f94da2f763c2 100644 --- a/llvm/lib/Analysis/InstructionSimplify.cpp +++ b/llvm/lib/Analysis/InstructionSimplify.cpp @@ -2686,27 +2686,15 @@ static Constant *computePointerICmp(CmpPredicate Pred, Value *LHS, Value *RHS, const DataLayout &DL = Q.DL; const TargetLibraryInfo *TLI = Q.TLI; - // We can only fold certain predicates on pointer comparisons. - switch (Pred) { - default: + // We fold equality and unsigned integer predicates on pointer comparisons, + // but forbid signed predicates since a GEP with inbounds could cross the sign + // boundary. + if (!CmpInst::isIntPredicate(Pred) || CmpInst::isSigned(Pred)) return nullptr; - // Equality comparisons are easy to fold. - case CmpInst::ICMP_EQ: - case CmpInst::ICMP_NE: - break; - - // We can only handle unsigned relational comparisons because 'inbounds' on - // a GEP only protects against unsigned wrapping. - case CmpInst::ICMP_UGT: - case CmpInst::ICMP_UGE: - case CmpInst::ICMP_ULT: - case CmpInst::ICMP_ULE: - // However, we have to switch them to their signed variants to handle - // negative indices from the base pointer. - Pred = ICmpInst::getSignedPredicate(Pred); - break; - } + // We have to switch to a signed predicate to handle negative indices from + // the base pointer. + Pred = ICmpInst::getSignedPredicate(Pred); // Strip off any constant offsets so that we can reason about them. // It's tempting to use getUnderlyingObject or even just stripInBoundsOffsets @@ -2730,7 +2718,7 @@ static Constant *computePointerICmp(CmpPredicate Pred, Value *LHS, Value *RHS, ICmpInst::compare(LHSOffset, RHSOffset, Pred)); // Various optimizations for (in)equality comparisons. - if (Pred == CmpInst::ICMP_EQ || Pred == CmpInst::ICMP_NE) { + if (ICmpInst::isEquality(Pred)) { // Different non-empty allocations that exist at the same time have // different addresses (if the program can tell). If the offsets are // within the bounds of their allocations (and not one-past-the-end! From b7fc2604dcf7e0d1d322506f51e2bf41654dccb1 Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Fri, 7 Feb 2025 16:28:13 +0000 Subject: [PATCH 2/2] IS: fix thinko --- llvm/lib/Analysis/InstructionSimplify.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/llvm/lib/Analysis/InstructionSimplify.cpp b/llvm/lib/Analysis/InstructionSimplify.cpp index 2f94da2f763c2..59002cd934ab1 100644 --- a/llvm/lib/Analysis/InstructionSimplify.cpp +++ b/llvm/lib/Analysis/InstructionSimplify.cpp @@ -2686,10 +2686,9 @@ static Constant *computePointerICmp(CmpPredicate Pred, Value *LHS, Value *RHS, const DataLayout &DL = Q.DL; const TargetLibraryInfo *TLI = Q.TLI; - // We fold equality and unsigned integer predicates on pointer comparisons, - // but forbid signed predicates since a GEP with inbounds could cross the sign - // boundary. - if (!CmpInst::isIntPredicate(Pred) || CmpInst::isSigned(Pred)) + // We fold equality and unsigned predicates on pointer comparisons, but forbid + // signed predicates since a GEP with inbounds could cross the sign boundary. + if (CmpInst::isSigned(Pred)) return nullptr; // We have to switch to a signed predicate to handle negative indices from