[PATCH] D92367: [SCEV] Fix incorrect exit count calculated in error scope

[Mindong Chen via Phabricator via llvm-commits]

mdchen added inline comments.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:7828
+  if (!IdxExpr || !IdxExpr->getLoop()->contains(L) || !IdxExpr->isAffine() ||
+      isLoopInvariant(IdxExpr, L) ||
       !isa<SCEVConstant>(IdxExpr->getOperand(0)) ||
----------------
mkazantsev wrote:
> mkazantsev wrote:
> > You require the `IdxExpr` be from a containing loop, which automatically makes  `IdxExpr` invariant w.r.t. `L`. So this whole condition is just trivially false. Is that really what you want to make?
> Sorry, the whole condition is trivially true. AddRecs from containing loops are always invariant w.r.t child loops.
> You require the IdxExpr be from a containing loop, which automatically makes IdxExpr invariant w.r.t. L

I see...Actually what I need is `IdxExpr->getLoop()==L`. Though in this case it won't exceed the scope `L` from where `Idx` is computed IIUC, I admit it's incorrect. Will do the change.


================
Comment at: llvm/test/Analysis/ScalarEvolution/incorrect-exit-count.ll:1
+; RUN: opt -analyze -scalar-evolution < %s | FileCheck %s
+
----------------
mkazantsev wrote:
> Please commit this test separately using auto-generated checks from ` utils/update_analyze_test_checks.py`. If you are addressing some bug from bugs.llvm.com, better name the test after this bug (like prXXXXX.ll).
> 
> Then, apply your changes and re-run  utils/update_analyze_test_checks.py on it. The diff will show what your patch has changed.
> 
> This test is very big. You could also try to use bugpoint tool to reduce the test (see https://llvm.org/docs/Bugpoint.html), because now I cannot figure what's going on here. Both test reduction (if possible) and explanatory comments (which block/loop are causing the problem and why) would be extremely useful to understand your fix.
I tried `llvm-reduce` but the output was not desirable, will add necessary comments, thanks! In short, the bug is that when computing the exit count of an outerloop's exiting block, the current `computeLoadConstantCompareExitLimit` method may choose an addrec of its child loop, which is incorrect.


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

https://reviews.llvm.org/D92367