diff mbox series

[V2,RFC] Fix PR62147 by passing finiteness information to RTL phase

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

Commit Message

Kewen.Lin June 25, 2019, 9:41 a.m. UTC 
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.

Comments

Jeff Law June 25, 2019, 9:49 p.m. UTC | #1 
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
Kewen.Lin June 26, 2019, 3:45 a.m. UTC | #2 
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
>
Kewen.Lin June 27, 2019, 5:37 a.m. UTC | #3 
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 mbox series

Patch

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} } } */