[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/test/Analysis/ScalarEvolution/incorrect-exit-count.ll:1
+; RUN: opt -analyze -scalar-evolution < %s | FileCheck %s
+
----------------
mkazantsev wrote:
> mdchen wrote:
> > mkazantsev wrote:
> > > mdchen wrote:
> > > > 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.
> > > Then at least do the first step: commit this test as is with auto-generated checks, and then rebase your patch on top of that to show what has changed.
> > Sorry, I don't understand well. Do you mean that I should separate it into two commits or only keep those changed checks? 
> I mean two commits.
> 
> First commit: this test, with no code changes, with auto-generated checks (use utils/update_analyze_test_checks.py to generate them).
> Second commit: your code changes + re-run of utils/update_analyze_test_checks.py to show what has changed.
I've submitted the test in D94657, could you review and commit it for me since I don't have the access right.


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

https://reviews.llvm.org/D92367