[llvm] [IR] Use iteration limit in stripAndAccumulateConstantOffsets (PR #190473)

[via llvm-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-llvm-ir

Author: Alexis Engelke (aengelke)

<details>
<summary>Changes</summary>

Using a SmallPtrSet is not free, even for small counts. The actual
iteration numbers can be quite high for loops with a high unroll count
-- UnrollLoop will create a long GEP chain and then InstSimplify will
call the function for every part of the chain. (This happens in multiple
occasions: e.g. computePointerICmp and simplifyGEPInst will both walk
the chain.) Therefore, even without endless loops, an iteration limit
seems appropriate here.

For Clang, the statistics are:

  47985 N=6..9
   4831 N=10..14
   4054 N=15..19
   3731 N=20..24
   3628 N=25..29
    705 N=30
    475 N=31
    242 N=32
     12 N=33
    ...
      4 N>100 (single function with a high unroll count)

Also add missing loop exits to CallBase and the default case, which
previously relied on not chaining V and going through the Visited set.


---
Full diff: https://github.com/llvm/llvm-project/pull/190473.diff


1 Files Affected:

- (modified) llvm/lib/IR/Value.cpp (+27-14) 


``````````diff
diff --git a/llvm/lib/IR/Value.cpp b/llvm/lib/IR/Value.cpp
index 360bf0f8fc47f..6627a32d35377 100644
--- a/llvm/lib/IR/Value.cpp
+++ b/llvm/lib/IR/Value.cpp
@@ -739,15 +739,20 @@ const Value *Value::stripAndAccumulateConstantOffsets(
          "The offset bit width does not match the DL specification.");
 
   // Even though we don't look through PHI nodes, we could be called on an
-  // instruction in an unreachable block, which may be on a cycle.
-  SmallPtrSet<const Value *, 4> Visited;
-  Visited.insert(this);
+  // instruction in an unreachable block, which may be on a cycle. E.g.:
+  //   %gep = getelementptr inbounds nuw i8, ptr %gep, i64 32
+  //
+  // Bound against that case with a simple iteration counter. The number of
+  // iterations can be quite high for very long GEP chains after loop unrolling.
+  // However, most cases take less than 30 iterations. The rare cases that have
+  // longer chains probably won't benefit from additional information...
   const Value *V = this;
-  do {
+  unsigned N = 0;
+  for (; N < 30; ++N) {
     if (auto *GEP = dyn_cast<GEPOperator>(V)) {
       // If in-bounds was requested, we do not strip non-in-bounds GEPs.
       if (!AllowNonInbounds && !GEP->isInBounds())
-        return V;
+        break;
 
       // If one of the values we have visited is an addrspacecast, then
       // the pointer type of this GEP may be different from the type
@@ -757,13 +762,13 @@ const Value *Value::stripAndAccumulateConstantOffsets(
       // pointer type.
       APInt GEPOffset(DL.getIndexTypeSizeInBits(V->getType()), 0);
       if (!GEP->accumulateConstantOffset(DL, GEPOffset, ExternalAnalysis))
-        return V;
+        break;
 
       // Stop traversal if the pointer offset wouldn't fit in the bit-width
       // provided by the Offset argument. This can happen due to AddrSpaceCast
       // stripping.
       if (GEPOffset.getSignificantBits() > BitWidth)
-        return V;
+        break;
 
       // External Analysis can return a result higher/lower than the value
       // represents. We need to detect overflow/underflow.
@@ -776,10 +781,14 @@ const Value *Value::stripAndAccumulateConstantOffsets(
         Offset = Offset.sadd_ov(GEPOffsetST, Overflow);
         if (Overflow) {
           Offset = std::move(OldOffset);
-          return V;
+          break;
         }
       }
-      V = GEP->getPointerOperand();
+      const Value *NewV = GEP->getPointerOperand();
+      // Quick exit for degenerate IR, which can happen in unreachable blocks.
+      if (NewV == V)
+        break;
+      V = NewV;
     } else if (Operator::getOpcode(V) == Instruction::BitCast ||
                Operator::getOpcode(V) == Instruction::AddrSpaceCast) {
       V = cast<Operator>(V)->getOperand(0);
@@ -789,29 +798,33 @@ const Value *Value::stripAndAccumulateConstantOffsets(
     } else if (const auto *Call = dyn_cast<CallBase>(V)) {
         if (const Value *RV = Call->getReturnedArgOperand())
           V = RV;
-        if (AllowInvariantGroup && Call->isLaunderOrStripInvariantGroup())
+        else if (AllowInvariantGroup && Call->isLaunderOrStripInvariantGroup())
           V = Call->getArgOperand(0);
+        else
+          break;
     } else if (auto *Int2Ptr = dyn_cast<Operator>(V)) {
       // Try to accumulate across (inttoptr (add (ptrtoint p), off)).
       if (!AllowNonInbounds || !LookThroughIntToPtr || !Int2Ptr ||
           Int2Ptr->getOpcode() != Instruction::IntToPtr ||
           Int2Ptr->getOperand(0)->getType()->getScalarSizeInBits() != BitWidth)
-        return V;
+        break;
 
       auto *Add = dyn_cast<AddOperator>(Int2Ptr->getOperand(0));
       if (!Add)
-        return V;
+        break;
 
       auto *Ptr2Int = dyn_cast<PtrToIntOperator>(Add->getOperand(0));
       auto *CI = dyn_cast<ConstantInt>(Add->getOperand(1));
       if (!Ptr2Int || !CI)
-        return V;
+        break;
 
       Offset += CI->getValue();
       V = Ptr2Int->getOperand(0);
+    } else {
+      break;
     }
     assert(V->getType()->isPtrOrPtrVectorTy() && "Unexpected operand type!");
-  } while (Visited.insert(V).second);
+  }
 
   return V;
 }

``````````

</details>


https://github.com/llvm/llvm-project/pull/190473