[PATCH] D103991: [SCEV] Extend mustprogress reasoning to ne exit tests

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:7918
+  //   caused this exit to be taken previously.  Thus, this exit is
+  //   dynamically dead.
+  // * If this is the sole exit, then a dead exit implies the loop
----------------
mkazantsev wrote:
> It is only true if this condition dominates the latch. If not, it might exit at some point during the iteration space, but it was guarded by a volatile condition that prevented it. If my understanding is correct, please specify this requirement explicitly in function's comment.
> 
> Maybe (I'm not sure) this still holds for non-dominating exits if it is the only exit from the loop.
All exits being analyzed for exit counts must dominate the latch.  See computeExitLimit.

I might be remembering this wrong, but I think you were even the person who added that code.  :)


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:7931
+
+  auto *Stride = IV->getStepRecurrence(*this);
+  auto *StrideC = dyn_cast<SCEVConstant>(Stride);
----------------
mkazantsev wrote:
> Please add early bail if IV is not affine. This will save compile time for complex addrecs.
I don't think it actually saves anything in this case.  Both callers filter out non-affine ARs before calling this.  I'll make it an assert though.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:7933
+  auto *StrideC = dyn_cast<SCEVConstant>(Stride);
+  if (!StrideC || !StrideC->getAPInt().isPowerOf2())
+    return SCEV::FlagAnyWrap;
----------------
mkazantsev wrote:
> How about negative powers of 2? I think all your reasoning validly applies for them too.
I believe it does too.  Incrementalism.  And not terrible relevant for LT/EQ cases, it'll be more important when applied to GT.  


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:7942
+
+  if (ICmpInst::isEquality(Pred))
+    return SCEV::FlagNW;
----------------
mkazantsev wrote:
> This should go before anything else to save compile time.
Er, I think you misread the code?  It's at the earliest point it can legally be.  It's proving no-self-wrap.  


================
Comment at: llvm/test/Analysis/ScalarEvolution/ne-overflow.ll:10
+
+; CHECK: Determining loop execution counts for: @test
+; CHECK: Loop %for.body: backedge-taken count is ((-2 + %N) /u 2)
----------------
mkazantsev wrote:
> Please use ./utils/update_analyze_test_checks.py. It works well with SCEV.
No actually, it doesn't.  It just claims to.  I've tried it with other test changes recently, and been very unpleasantly surprised.  It mostly just results in missing check lines.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103991/new/

https://reviews.llvm.org/D103991