Message ID | 17421a36-c7bc-ef9e-b3df-e262eec012c8@linux.ibm.com |
---|---|
State | New |
Headers | show |
Series | [V2,RFC] Fix PR62147 by passing finiteness information to RTL phase | expand |
On 6/25/19 3:41 AM, Kewen.Lin wrote: > Hi Richard, > > Thanks a lot for review comments. > > on 2019/6/25 下午3:23, Richard Biener wrote: >> On Tue, 25 Jun 2019, Kewen.Lin wrote: >> >>> Hi all, >>> >>> >>> It's based on two observations: >>> 1) the loop structure for one specific loop is shared between middle-end and >>> back-end. >>> 2) for one specific loop, if it's finite then never become infinite itself. >>> >>> As one gcc newbie, I'm not sure whether these two observations are true in all >>> cases. Please feel free to correct me if anything missing. >> >> I think 2) is not true with -ffinite-loops. > > I just looked at the patch on this option, I don't fully understand it can affect > 2). It's to take one loop as finite with any normal exit, can some loop with this > assertion turn into infinite later by some other analysis? > >> >>> btw, I also took a look at how the loop constraint LOOP_C_FINITE is used, I think >>> it's not suitable for this purpose, it's mainly set by vectorizer and tell niter >>> and scev to take one loop as finite. The original patch has the words >>> "constraint flag is mainly set by consumers and affects certain semantics of >>> niter analyzer APIs". >>> >>> Bootstrapped and regression testing passed on powerpc64le-unknown-linux-gnu. >> >> Did you consider to simply use finite_loop_p () from doloop.c? That >> would be a much simpler patch. > > Good suggestion! I took it for granted that the function can be only efficient in > middle-end, but actually some information like bit any_upper_bound could be kept to > RTL. > >> >> For the testcase in question -ffinite-loops would provide this guarantee >> even on RTL, so would the upper bound that may be still set. >> >> Richard. >> > > The new version with Richard's suggestion listed below. > Regression testing is ongoing. > > > Thanks, > Kewen > > ----------- > > gcc/ChangeLog > > 2019-06-25 Kewen Lin <linkw@gcc.gnu.org> > > PR target/62147 > * gcc/loop-iv.c (find_simple_exit): Call finite_loop_p to update finiteness. > > gcc/testsuite/ChangeLog > > 2019-06-25 Kewen Lin <linkw@gcc.gnu.org> > > PR target/62147 > * gcc.target/powerpc/pr62147.c: New test. This is fine assuming regression testing was OK. One might argue that "finite_loop_p" belongs elsewhere since it's not really querying tree/gimple structures. jeff
Hi Jeff, on 2019/6/26 上午5:49, Jeff Law wrote: > On 6/25/19 3:41 AM, Kewen.Lin wrote: >> Hi Richard, >> >> Thanks a lot for review comments. >> >> on 2019/6/25 下午3:23, Richard Biener wrote: >>> On Tue, 25 Jun 2019, Kewen.Lin wrote: >>> >>>> Hi all, >>>> >>>> >>>> It's based on two observations: >>>> 1) the loop structure for one specific loop is shared between middle-end and >>>> back-end. >>>> 2) for one specific loop, if it's finite then never become infinite itself. >>>> >>>> As one gcc newbie, I'm not sure whether these two observations are true in all >>>> cases. Please feel free to correct me if anything missing. >>> >>> I think 2) is not true with -ffinite-loops. >> >> I just looked at the patch on this option, I don't fully understand it can affect >> 2). It's to take one loop as finite with any normal exit, can some loop with this >> assertion turn into infinite later by some other analysis? >> >>> >>>> btw, I also took a look at how the loop constraint LOOP_C_FINITE is used, I think >>>> it's not suitable for this purpose, it's mainly set by vectorizer and tell niter >>>> and scev to take one loop as finite. The original patch has the words >>>> "constraint flag is mainly set by consumers and affects certain semantics of >>>> niter analyzer APIs". >>>> >>>> Bootstrapped and regression testing passed on powerpc64le-unknown-linux-gnu. >>> >>> Did you consider to simply use finite_loop_p () from doloop.c? That >>> would be a much simpler patch. >> >> Good suggestion! I took it for granted that the function can be only efficient in >> middle-end, but actually some information like bit any_upper_bound could be kept to >> RTL. >> >>> >>> For the testcase in question -ffinite-loops would provide this guarantee >>> even on RTL, so would the upper bound that may be still set. >>> >>> Richard. >>> >> >> The new version with Richard's suggestion listed below. >> Regression testing is ongoing. >> >> >> Thanks, >> Kewen >> >> ----------- >> >> gcc/ChangeLog >> >> 2019-06-25 Kewen Lin <linkw@gcc.gnu.org> >> >> PR target/62147 >> * gcc/loop-iv.c (find_simple_exit): Call finite_loop_p to update finiteness. >> >> gcc/testsuite/ChangeLog >> >> 2019-06-25 Kewen Lin <linkw@gcc.gnu.org> >> >> PR target/62147 >> * gcc.target/powerpc/pr62147.c: New test. > This is fine assuming regression testing was OK. > Thanks Jeff! Bootstrapped and regression testing passed on powerpc64le-unknown-linux-gnu. > One might argue that "finite_loop_p" belongs elsewhere since it's not > really querying tree/gimple structures. I guess it will do something gimple specific (estimate_numbers_of_iterations) when it can so it was placed there. Thanks, Kewen > > jeff >
Hi all, I've committed this and with one more change. --- gcc/loop-iv.c (revision 272731) +++ gcc/loop-iv.c (working copy) @@ -3004,7 +3004,7 @@ find_simple_exit (struct loop *loop, struct niter_ well. It results in incorrect predicate information on the exit condition expression. For example, if says [(int) _1 + -8, + , -8] != 0 finite, it means _1 can exactly divide -8. */ - if (single_exit (loop) && finite_loop_p (loop)) + if (desc->infinite && single_exit (loop) && finite_loop_p (loop)) { desc->infinite = NULL_RTX; if (dump_file) on 2019/6/26 上午11:45, Kewen.Lin wrote: > Hi Jeff, > > on 2019/6/26 上午5:49, Jeff Law wrote: >> On 6/25/19 3:41 AM, Kewen.Lin wrote: >>> Hi Richard, >>> >>> Thanks a lot for review comments. >>> >>> on 2019/6/25 下午3:23, Richard Biener wrote: >>>> On Tue, 25 Jun 2019, Kewen.Lin wrote: >>>> >>>>> Hi all, >>>>> >>>>> >>>>> It's based on two observations: >>>>> 1) the loop structure for one specific loop is shared between middle-end and >>>>> back-end. >>>>> 2) for one specific loop, if it's finite then never become infinite itself. >>>>> >>>>> As one gcc newbie, I'm not sure whether these two observations are true in all >>>>> cases. Please feel free to correct me if anything missing. >>>> >>>> I think 2) is not true with -ffinite-loops. >>> >>> I just looked at the patch on this option, I don't fully understand it can affect >>> 2). It's to take one loop as finite with any normal exit, can some loop with this >>> assertion turn into infinite later by some other analysis? >>> >>>> >>>>> btw, I also took a look at how the loop constraint LOOP_C_FINITE is used, I think >>>>> it's not suitable for this purpose, it's mainly set by vectorizer and tell niter >>>>> and scev to take one loop as finite. The original patch has the words >>>>> "constraint flag is mainly set by consumers and affects certain semantics of >>>>> niter analyzer APIs". >>>>> >>>>> Bootstrapped and regression testing passed on powerpc64le-unknown-linux-gnu. >>>> >>>> Did you consider to simply use finite_loop_p () from doloop.c? That >>>> would be a much simpler patch. >>> >>> Good suggestion! I took it for granted that the function can be only efficient in >>> middle-end, but actually some information like bit any_upper_bound could be kept to >>> RTL. >>> >>>> >>>> For the testcase in question -ffinite-loops would provide this guarantee >>>> even on RTL, so would the upper bound that may be still set. >>>> >>>> Richard. >>>> >>> >>> The new version with Richard's suggestion listed below. >>> Regression testing is ongoing. >>> >>> >>> Thanks, >>> Kewen >>> >>> ----------- >>> >>> gcc/ChangeLog >>> >>> 2019-06-25 Kewen Lin <linkw@gcc.gnu.org> >>> >>> PR target/62147 >>> * gcc/loop-iv.c (find_simple_exit): Call finite_loop_p to update finiteness. >>> >>> gcc/testsuite/ChangeLog >>> >>> 2019-06-25 Kewen Lin <linkw@gcc.gnu.org> >>> >>> PR target/62147 >>> * gcc.target/powerpc/pr62147.c: New test. >> This is fine assuming regression testing was OK. >> > > Thanks Jeff! Bootstrapped and regression testing passed on powerpc64le-unknown-linux-gnu. > >> One might argue that "finite_loop_p" belongs elsewhere since it's not >> really querying tree/gimple structures. > > I guess it will do something gimple specific (estimate_numbers_of_iterations) when it can > so it was placed there. > > > Thanks, > Kewen > >> >> jeff >> >
diff --git a/gcc/loop-iv.c b/gcc/loop-iv.c index 82b4bdb1523..36f9856f5f6 100644 --- a/gcc/loop-iv.c +++ b/gcc/loop-iv.c @@ -61,6 +61,7 @@ along with GCC; see the file COPYING3. If not see #include "intl.h" #include "dumpfile.h" #include "rtl-iter.h" +#include "tree-ssa-loop-niter.h" /* Possible return values of iv_get_reaching_def. */ @@ -2997,6 +2998,19 @@ find_simple_exit (struct loop *loop, struct niter_desc *desc) fprintf (dump_file, "Loop %d is not simple.\n", loop->num); } + /* Fix up the finiteness if possible. We can only do it for single exit, + since the loop is finite, but it's possible that we predicate one loop + exit to be finite which can not be determined as finite in middle-end as + well. It results in incorrect predicate information on the exit condition + expression. For example, if says [(int) _1 + -8, + , -8] != 0 finite, + it means _1 can exactly divide -8. */ + if (single_exit (loop) && finite_loop_p (loop)) + { + desc->infinite = NULL_RTX; + if (dump_file) + fprintf (dump_file, " infinite updated to finite.\n"); + } + free (body); } diff --git a/gcc/testsuite/gcc.target/powerpc/pr62147.c b/gcc/testsuite/gcc.target/powerpc/pr62147.c new file mode 100644 index 00000000000..635c73711da --- /dev/null +++ b/gcc/testsuite/gcc.target/powerpc/pr62147.c @@ -0,0 +1,24 @@ +/* { dg-do compile { target { powerpc*-*-* } } } */ +/* { dg-options "-O2 -fno-tree-loop-distribute-patterns" } */ + +/* Note that it's required to disable loop-distribute-patterns, otherwise the + loop will be optimized to memset. */ + +/* Expect loop_iv can know the loop is finite so the doloop_optimize + can perform the doloop transformation. */ + +typedef struct { + int l; + int b[258]; +} S; + +void clear (S* s ) +{ + int i; + int len = s->l + 1; + + for (i = 0; i <= len; i++) + s->b[i] = 0; +} + +/* { dg-final { scan-assembler {\mbdnz\M} } } */