[llvm] 69c4346 - [LoopUnrollAnalyzer] Don't simplify signed pointer comparison

[Nikita Popov via llvm-commits]

Author: Nikita Popov
Date: 2024-08-28T12:15:12+02:00
New Revision: 69c43468d3f21df6232fda0530f03f18b0f40345

URL: https://github.com/llvm/llvm-project/commit/69c43468d3f21df6232fda0530f03f18b0f40345
DIFF: https://github.com/llvm/llvm-project/commit/69c43468d3f21df6232fda0530f03f18b0f40345.diff

LOG: [LoopUnrollAnalyzer] Don't simplify signed pointer comparison

We're generally not able to simplify signed pointer comparisons
(because we don't have no-wrap flags that would permit it), so
we shouldn't pretend that we can in the cost model.

The unsigned comparison case is also not modelled correctly,
as explained in the added comment. As this is a cost model
inaccuracy at worst, I'm leaving it alone for now.

Added: 
    

Modified: 
    llvm/lib/Analysis/LoopUnrollAnalyzer.cpp
    llvm/unittests/Analysis/UnrollAnalyzerTest.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Analysis/LoopUnrollAnalyzer.cpp b/llvm/lib/Analysis/LoopUnrollAnalyzer.cpp
index 4b8f5d7543f78b..9c78e2afaede71 100644
--- a/llvm/lib/Analysis/LoopUnrollAnalyzer.cpp
+++ b/llvm/lib/Analysis/LoopUnrollAnalyzer.cpp
@@ -155,7 +155,7 @@ bool UnrolledInstAnalyzer::visitCmpInst(CmpInst &I) {
     if (Value *SimpleRHS = SimplifiedValues.lookup(RHS))
       RHS = SimpleRHS;
 
-  if (!isa<Constant>(LHS) && !isa<Constant>(RHS)) {
+  if (!isa<Constant>(LHS) && !isa<Constant>(RHS) && !I.isSigned()) {
     auto SimplifiedLHS = SimplifiedAddresses.find(LHS);
     if (SimplifiedLHS != SimplifiedAddresses.end()) {
       auto SimplifiedRHS = SimplifiedAddresses.find(RHS);
@@ -163,6 +163,11 @@ bool UnrolledInstAnalyzer::visitCmpInst(CmpInst &I) {
         SimplifiedAddress &LHSAddr = SimplifiedLHS->second;
         SimplifiedAddress &RHSAddr = SimplifiedRHS->second;
         if (LHSAddr.Base == RHSAddr.Base) {
+          // FIXME: This is only correct for equality predicates. For
+          // unsigned predicates, this only holds if we have nowrap flags,
+          // which we don't track (for nuw it's valid as-is, for nusw it
+          // requires converting the predicated to signed). As this is used only
+          // for cost modelling, this is not a correctness issue.
           bool Res = ICmpInst::compare(LHSAddr.Offset, RHSAddr.Offset,
                                        I.getPredicate());
           SimplifiedValues[&I] = ConstantInt::getBool(I.getType(), Res);

diff  --git a/llvm/unittests/Analysis/UnrollAnalyzerTest.cpp b/llvm/unittests/Analysis/UnrollAnalyzerTest.cpp
index 721d67f22f2f27..0ff08d19957efc 100644
--- a/llvm/unittests/Analysis/UnrollAnalyzerTest.cpp
+++ b/llvm/unittests/Analysis/UnrollAnalyzerTest.cpp
@@ -221,6 +221,8 @@ TEST(UnrollAnalyzerTest, PtrCmpSimplifications) {
       "  %iv.0 = phi i8* [ %a, %entry ], [ %iv.1, %loop.body ]\n"
       "  %iv2.0 = phi i8* [ %start.iv2, %entry ], [ %iv2.1, %loop.body ]\n"
       "  %cmp = icmp eq i8* %iv2.0, %iv.0\n"
+      "  %cmp2 = icmp slt i8* %iv2.0, %iv.0\n"
+      "  %cmp3 = icmp ult i8* %iv2.0, %iv.0\n"
       "  %iv.1 = getelementptr inbounds i8, i8* %iv.0, i64 1\n"
       "  %iv2.1 = getelementptr inbounds i8, i8* %iv2.0, i64 1\n"
       "  %exitcond = icmp ne i8* %iv.1, %limit\n"
@@ -242,12 +244,21 @@ TEST(UnrollAnalyzerTest, PtrCmpSimplifications) {
 
   BasicBlock::iterator BBI = Header->begin();
   std::advance(BBI, 2);
-  Instruction *Y1 = &*BBI;
+  Instruction *Cmp1 = &*BBI++;
+  Instruction *Cmp2 = &*BBI++;
+  Instruction *Cmp3 = &*BBI++;
   // Check simplification expected on the 5th iteration.
   // Check that "%cmp = icmp eq i8* %iv2.0, %iv.0" is simplified to 0.
-  auto I1 = SimplifiedValuesVector[5].find(Y1);
+  auto I1 = SimplifiedValuesVector[5].find(Cmp1);
   EXPECT_TRUE(I1 != SimplifiedValuesVector[5].end());
   EXPECT_EQ(cast<ConstantInt>((*I1).second)->getZExtValue(), 0U);
+  // Check that "%cmp2 = icmp slt i8* %iv2.0, %iv.0" does not simplify
+  auto I2 = SimplifiedValuesVector[5].find(Cmp2);
+  EXPECT_TRUE(I2 == SimplifiedValuesVector[5].end());
+  // Check that "%cmp3 = icmp ult i8* %iv2.0, %iv.0" is simplified to 0.
+  auto I3 = SimplifiedValuesVector[5].find(Cmp3);
+  EXPECT_TRUE(I3 != SimplifiedValuesVector[5].end());
+  EXPECT_EQ(cast<ConstantInt>((*I1).second)->getZExtValue(), 0U);
 }
 
 TEST(UnrollAnalyzerTest, CastSimplifications) {