[llvm] [LAA][NFC] Refactor deref no-wrap check; expose broken reverse-loop bounds (PR #211960)

[Antonio Frighetto via llvm-commits]

================
@@ -277,45 +281,51 @@ static bool evaluatePtrAddRecAtMaxBTCWillNotWrap(
     LoopGuards.emplace(ScalarEvolution::LoopGuards::collect(AR->getLoop(), SE));
   MaxBTC = SE.applyLoopGuards(MaxBTC, *LoopGuards);
 
-  const SCEV *OffsetAtLastIter =
-      mulSCEVNoOverflow(MaxBTC, SE.getAbsExpr(Step, /*IsNSW=*/false), SE);
-  if (!OffsetAtLastIter) {
+  const SCEV *AbsStep = SE.getAbsExpr(Step, /*IsNSW=*/false);
+  // Total distance (in bytes) walked between the first and the last
+  // accessed pointer; MaxBTC * |Step|.
+  const SCEV *WalkBytes = mulSCEVNoOverflow(MaxBTC, AbsStep, SE);
+  if (!WalkBytes) {
     // Re-try with constant max backedge-taken count if using the symbolic one
     // failed.
     MaxBTC = SE.getConstantMaxBackedgeTakenCount(AR->getLoop());
     if (isa<SCEVCouldNotCompute>(MaxBTC))
       return false;
-    MaxBTC = SE.getNoopOrZeroExtend(
-        MaxBTC, WiderTy);
-    OffsetAtLastIter =
-        mulSCEVNoOverflow(MaxBTC, SE.getAbsExpr(Step, /*IsNSW=*/false), SE);
-    if (!OffsetAtLastIter)
+    MaxBTC = SE.getNoopOrZeroExtend(MaxBTC, WiderTy);
+    WalkBytes = mulSCEVNoOverflow(MaxBTC, AbsStep, SE);
+    if (!WalkBytes)
       return false;
   }
 
-  const SCEV *OffsetEndBytes = addSCEVNoOverflow(
-      OffsetAtLastIter, SE.getNoopOrZeroExtend(EltSize, WiderTy), SE);
-  if (!OffsetEndBytes)
+  // Total length in bytes of the accessed range (from the first accessed
+  // byte through the end of the last access); WalkBytes + EltSize.
+  const SCEV *SpanBytes = addSCEVNoOverflow(
+      WalkBytes, SE.getNoopOrZeroExtend(EltSize, WiderTy), SE);
+  if (!SpanBytes)
     return false;
 
+  // Compute MaxOffset per direction: exclusive upper offset of the
+  // accessed range.
+  const SCEV *MaxOffset;
   if (IsKnownNonNegative) {
-    // For positive steps, check if
-    //  (AR->getStart() - StartPtr) + (MaxBTC  * Step) + EltSize <= DerefBytes,
-    // while making sure none of the computations unsigned wrap themselves.
-    const SCEV *EndBytes = addSCEVNoOverflow(StartOffset, OffsetEndBytes, SE);
-    if (!EndBytes)
+    MaxOffset = addSCEVNoOverflow(StartOffset, SpanBytes, SE);
+    if (!MaxOffset)
       return false;
-
     DerefBytesSCEV = SE.applyLoopGuards(DerefBytesSCEV, *LoopGuards);
-    return SE.isKnownPredicate(CmpInst::ICMP_ULE, EndBytes, DerefBytesSCEV);
+  } else {
+    // FIXME: two independent off-by-EltSize bugs on this branch:
+    //  1. StartOffset here is actually the HIGHEST offset, because it is
+    //     computed from AR->getStart() rather than
+    //     AR->evaluateAtIteration(MaxBTC, SE) (see FIXME above).
+    //  2. The lower check is over-strict by EltSize and the upper is
+    //     under-counted by EltSize.
+    assert(SE.isKnownNegative(Step) && "must be known negative");
+    if (!SE.isKnownPredicate(CmpInst::ICMP_SGE, StartOffset, SpanBytes))
----------------
antoniofrighetto wrote:

Okay, so, given the dereferenceable region is [StartPtr, StartPtr + DerefBytes), if I'm understanding correctly, ensuring `StartOffset >= WalkBytes` would make the lower bound correct for negative downward steps (so that we wouldn't underflow past `StartPtr`), but we would still be missing an upper bound, as the existing code is (incorrectly) asking SCEV whether `StartOffset <= DerefBytes` holds, instead of asking whether `StartOffset + EltSize <= DerefBytes` holds (and the former leads to exceeding the deref region).

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