LAA: strip unnecessary getUniqueCastUse

[artagnon]

733b8b2 ([LAA] Simplify identification of speculatable strides [nfc]) refactored getStrideFromPointer() to compute directly on SCEVs, and return an SCEV expression instead of a Value. However, it left behind a call to getUniqueCastUse(), which is completely unnecessary. Remove this, showing a positive test update, and simplify the surrounding program logic.

[llvmbot]

@llvm/pr-subscribers-llvm-analysis

Author: Ramkumar Ramachandra (artagnon)

Changes

733b8b2 ([LAA] Simplify identification of speculatable strides [nfc]) refactored getStrideFromPointer() to compute directly on SCEVs, and return an SCEV expression instead of a Value. However, it left behind a call to getUniqueCastUse(), which is completely unnecessary. Remove this dead code, and simplify the surrounding program logic.

---

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

1 Files Affected:

• (modified) llvm/lib/Analysis/LoopAccessAnalysis.cpp (+7-29)

diff --git a/llvm/lib/Analysis/LoopAccessAnalysis.cpp b/llvm/lib/Analysis/LoopAccessAnalysis.cpp
index d071e53324408..2574da76e747b 100644
--- a/llvm/lib/Analysis/LoopAccessAnalysis.cpp
+++ b/llvm/lib/Analysis/LoopAccessAnalysis.cpp
@@ -2656,7 +2656,7 @@ void LoopAccessInfo::analyzeLoop(AAResults *AA, LoopInfo *LI,
                                SymbolicStrides, UncomputablePtr, false);
   if (!CanDoRTIfNeeded) {
     auto *I = dyn_cast_or_null<Instruction>(UncomputablePtr);
-    recordAnalysis("CantIdentifyArrayBounds", I) 
+    recordAnalysis("CantIdentifyArrayBounds", I)
         << "cannot identify array bounds";
     LLVM_DEBUG(dbgs() << "LAA: We can't vectorize because we can't find "
                       << "the array bounds.\n");
@@ -2873,21 +2873,6 @@ static Value *stripGetElementPtr(Value *Ptr, ScalarEvolution *SE, Loop *Lp) {
   return GEP->getOperand(InductionOperand);
 }
 
-/// If a value has only one user that is a CastInst, return it.
-static Value *getUniqueCastUse(Value *Ptr, Loop *Lp, Type *Ty) {
-  Value *UniqueCast = nullptr;
-  for (User *U : Ptr->users()) {
-    CastInst *CI = dyn_cast<CastInst>(U);
-    if (CI && CI->getType() == Ty) {
-      if (!UniqueCast)
-        UniqueCast = CI;
-      else
-        return nullptr;
-    }
-  }
-  return UniqueCast;
-}
-
 /// Get the stride of a pointer access in a loop. Looks for symbolic
 /// strides "a[i*stride]". Returns the symbolic stride, or null otherwise.
 static const SCEV *getStrideFromPointer(Value *Ptr, ScalarEvolution *SE, Loop *Lp) {
@@ -2950,21 +2935,14 @@ static const SCEV *getStrideFromPointer(Value *Ptr, ScalarEvolution *SE, Loop *L
     return nullptr;
 
   // Look for the loop invariant symbolic value.
-  const SCEVUnknown *U = dyn_cast<SCEVUnknown>(V);
-  if (!U) {
-    const auto *C = dyn_cast<SCEVIntegralCastExpr>(V);
-    if (!C)
-      return nullptr;
-    U = dyn_cast<SCEVUnknown>(C->getOperand());
-    if (!U)
-      return nullptr;
+  if (isa<SCEVUnknown>(V))
+    return V;
 
-    // Match legacy behavior - this is not needed for correctness
-    if (!getUniqueCastUse(U->getValue(), Lp, V->getType()))
-      return nullptr;
-  }
+  if (const auto *C = dyn_cast<SCEVIntegralCastExpr>(V))
+    if (isa<SCEVUnknown>(C->getOperand()))
+      return V;
 
-  return V;
+  return nullptr;
 }
 
 void LoopAccessInfo::collectStridedAccess(Value *MemAccess) {

[nikic]

This looks reasonable to me, but is it really NFC? I would expect this to allow determining the stride in more cases that previously failed that check?

[fhahn]

This looks reasonable to me, but is it really NFC? I would expect this to allow determining the stride in more cases that previously failed that check?

yeah this looks like we are missing test coverage

[artagnon]

Thanks for sanity-checking this: I previously didn't manage to cook up a sensible test. Now, the new tests are in #92253, and I've updated this patch to show the test update.

[artagnon]

Gentle ping. Since we've agreed that the change is good, can we verify that the test update is good as well?

[artagnon]

Rebased.

[artagnon]

Gentle ping.

[artagnon]

Gentle ping.

[artagnon]

Gentle ping.

[nikic]

LGTM. I don't see any obvious reason why profitability here should depend on whether the cast is unique or not.

[dtcxzyw]

This patch causes SCEVExpander to emit more ident.check checks when building openblas. Is it expected?

See also dtcxzyw/llvm-opt-benchmark#743 and dtcxzyw/llvm-opt-benchmark@6844857.

[artagnon]

This is unexpected. Let me take a look.

[artagnon]

Hi @dtcxzyw, thanks a lot for reporting the bug, and sorry for taking such a long time to reduce it. The regression is in loop-versioning, and I've opened #96656 to invetigate further.