[PR68906]
diff mbox

Message ID CAFiYyc0jmCTdKvKOZK-vZQ6c=2kE_VODDwk06XH1-tS1AO367w@mail.gmail.com
State New
Headers show

Commit Message

Richard Biener Dec. 16, 2015, 12:51 p.m. UTC
On Wed, Dec 16, 2015 at 1:14 PM, Yuri Rumyantsev <ysrumyan@gmail.com> wrote:
> Hi All,
>
> Here is simple patch which cures the issue with outer-loop unswitching
> - added invocation of number_of_latch_executions() to reject
> unswitching for non-iterated loops.
>
> Bootstrapping and regression testing did not show any new failures.
> Is it OK for trunk?

No, that looks like just papering over the issue.

The issue (with the 2nd testcase at least) is that single_exit () accepts
an exit from the inner loop.


fixes the runtime testcase for me (not suitable for the testsuite due
to the infinite
looping though).

Can you please bootstrap/test the above with your testcase?  The above patch is
ok if it passes testing (no time myself right now)

Thanks,
Richard.

> ChangeLog:
>
> 2014-12-16  Yuri Rumyantsev  <ysrumyan@gmail.com>
>
> PR tree-optimization/68906
> * tree-ssa-loop-unswitch.c : Include couple header files.
> (tree_unswitch_outer_loop): Use number_of_latch_executions
> to reject non-iterated loops.
>
> gcc/testsuite/ChangeLog
> * gcc.dg/torture/pr68906.c: New test.

Comments

Yuri Rumyantsev Dec. 16, 2015, 2:36 p.m. UTC | #1
Richard,

Here is updated patch which includes (1) a test on exit proposed by
you and (2) another test from PR68021 which is caught by new check on
counted loop. Outer-loop unswitching is not performed for both new
tests.

Bootstrapping and regression testing did not show any new failures.

Is it OK for trunk.

ChangeLog:
2014-12-16  Yuri Rumyantsev  <ysrumyan@gmail.com>

PR tree-optimization/68021
PR tree-optimization/68906
* tree-ssa-loop-unswitch.c : Include couple header files.
(tree_unswitch_outer_loop): Add check that an exit is not inside inner
loop, use number_of_latch_executions to detect non-iterated loops.

gcc/testsuite/ChangeLog
* gcc.dg/torture/pr68021.c: New test.
* gcc.dg/torture/pr68906.c: Likewise.

2015-12-16 15:51 GMT+03:00 Richard Biener <richard.guenther@gmail.com>:
> On Wed, Dec 16, 2015 at 1:14 PM, Yuri Rumyantsev <ysrumyan@gmail.com> wrote:
>> Hi All,
>>
>> Here is simple patch which cures the issue with outer-loop unswitching
>> - added invocation of number_of_latch_executions() to reject
>> unswitching for non-iterated loops.
>>
>> Bootstrapping and regression testing did not show any new failures.
>> Is it OK for trunk?
>
> No, that looks like just papering over the issue.
>
> The issue (with the 2nd testcase at least) is that single_exit () accepts
> an exit from the inner loop.
>
> Index: gcc/tree-ssa-loop-unswitch.c
> ===================================================================
> --- gcc/tree-ssa-loop-unswitch.c        (revision 231686)
> +++ gcc/tree-ssa-loop-unswitch.c        (working copy)
> @@ -431,7 +431,7 @@ tree_unswitch_outer_loop (struct loop *l
>      return false;
>    /* Accept loops with single exit only.  */
>    exit = single_exit (loop);
> -  if (!exit)
> +  if (!exit || exit->src->loop_father != loop)
>      return false;
>    /* Check that phi argument of exit edge is not defined inside loop.  */
>    if (!check_exit_phi (loop))
>
> fixes the runtime testcase for me (not suitable for the testsuite due
> to the infinite
> looping though).
>
> Can you please bootstrap/test the above with your testcase?  The above patch is
> ok if it passes testing (no time myself right now)
>
> Thanks,
> Richard.
>
>> ChangeLog:
>>
>> 2014-12-16  Yuri Rumyantsev  <ysrumyan@gmail.com>
>>
>> PR tree-optimization/68906
>> * tree-ssa-loop-unswitch.c : Include couple header files.
>> (tree_unswitch_outer_loop): Use number_of_latch_executions
>> to reject non-iterated loops.
>>
>> gcc/testsuite/ChangeLog
>> * gcc.dg/torture/pr68906.c: New test.
Richard Biener Dec. 16, 2015, 2:49 p.m. UTC | #2
On Wed, Dec 16, 2015 at 3:36 PM, Yuri Rumyantsev <ysrumyan@gmail.com> wrote:
> Richard,
>
> Here is updated patch which includes (1) a test on exit proposed by
> you and (2) another test from PR68021 which is caught by new check on
> counted loop. Outer-loop unswitching is not performed for both new
> tests.

As said I don't think

   /* If the loop is not expected to iterate, there is no need
       for unswitching.  */
-  iterations = estimated_loop_iterations_int (loop);
-  if (iterations >= 0 && iterations <= 1)
+  niters = number_of_latch_executions (loop);
+  if (!niters || chrec_contains_undetermined (niters))
     {

is good.  We do want to retain the estimated_loop_iterations check
as it takes into account profile data while yours lets through all
counted loops.

Also I don't see why SCEV needs to be able to analyze the IV to check
for validity.

Can you please split the patch into the part I suggested (which is ok)
and the rest?

Thanks,
Richard.

>
> Bootstrapping and regression testing did not show any new failures.
>
> Is it OK for trunk.
>
> ChangeLog:
> 2014-12-16  Yuri Rumyantsev  <ysrumyan@gmail.com>
>
> PR tree-optimization/68021
> PR tree-optimization/68906
> * tree-ssa-loop-unswitch.c : Include couple header files.
> (tree_unswitch_outer_loop): Add check that an exit is not inside inner
> loop, use number_of_latch_executions to detect non-iterated loops.
>
> gcc/testsuite/ChangeLog
> * gcc.dg/torture/pr68021.c: New test.
> * gcc.dg/torture/pr68906.c: Likewise.
>
> 2015-12-16 15:51 GMT+03:00 Richard Biener <richard.guenther@gmail.com>:
>> On Wed, Dec 16, 2015 at 1:14 PM, Yuri Rumyantsev <ysrumyan@gmail.com> wrote:
>>> Hi All,
>>>
>>> Here is simple patch which cures the issue with outer-loop unswitching
>>> - added invocation of number_of_latch_executions() to reject
>>> unswitching for non-iterated loops.
>>>
>>> Bootstrapping and regression testing did not show any new failures.
>>> Is it OK for trunk?
>>
>> No, that looks like just papering over the issue.
>>
>> The issue (with the 2nd testcase at least) is that single_exit () accepts
>> an exit from the inner loop.
>>
>> Index: gcc/tree-ssa-loop-unswitch.c
>> ===================================================================
>> --- gcc/tree-ssa-loop-unswitch.c        (revision 231686)
>> +++ gcc/tree-ssa-loop-unswitch.c        (working copy)
>> @@ -431,7 +431,7 @@ tree_unswitch_outer_loop (struct loop *l
>>      return false;
>>    /* Accept loops with single exit only.  */
>>    exit = single_exit (loop);
>> -  if (!exit)
>> +  if (!exit || exit->src->loop_father != loop)
>>      return false;
>>    /* Check that phi argument of exit edge is not defined inside loop.  */
>>    if (!check_exit_phi (loop))
>>
>> fixes the runtime testcase for me (not suitable for the testsuite due
>> to the infinite
>> looping though).
>>
>> Can you please bootstrap/test the above with your testcase?  The above patch is
>> ok if it passes testing (no time myself right now)
>>
>> Thanks,
>> Richard.
>>
>>> ChangeLog:
>>>
>>> 2014-12-16  Yuri Rumyantsev  <ysrumyan@gmail.com>
>>>
>>> PR tree-optimization/68906
>>> * tree-ssa-loop-unswitch.c : Include couple header files.
>>> (tree_unswitch_outer_loop): Use number_of_latch_executions
>>> to reject non-iterated loops.
>>>
>>> gcc/testsuite/ChangeLog
>>> * gcc.dg/torture/pr68906.c: New test.
Yuri Rumyantsev Dec. 17, 2015, 9:21 a.m. UTC | #3
Richard,

Here is modified patch which checks only that exit block belongs to loop.

Bootstrapping and regression testing were successful.
Is it OK for trunk?

ChangeLog:
2014-12-17  Yuri Rumyantsev  <ysrumyan@gmail.com>

PR tree-optimization/68906
* tree-ssa-loop-unswitch.c (tree_unswitch_outer_loop): Add  a check
that an exit block belongs to LOOP.

gcc/testsuite/ChangeLog
* gcc.dg/torture/pr68906.c: New test.
.

2015-12-16 17:49 GMT+03:00 Richard Biener <richard.guenther@gmail.com>:
> On Wed, Dec 16, 2015 at 3:36 PM, Yuri Rumyantsev <ysrumyan@gmail.com> wrote:
>> Richard,
>>
>> Here is updated patch which includes (1) a test on exit proposed by
>> you and (2) another test from PR68021 which is caught by new check on
>> counted loop. Outer-loop unswitching is not performed for both new
>> tests.
>
> As said I don't think
>
>    /* If the loop is not expected to iterate, there is no need
>        for unswitching.  */
> -  iterations = estimated_loop_iterations_int (loop);
> -  if (iterations >= 0 && iterations <= 1)
> +  niters = number_of_latch_executions (loop);
> +  if (!niters || chrec_contains_undetermined (niters))
>      {
>
> is good.  We do want to retain the estimated_loop_iterations check
> as it takes into account profile data while yours lets through all
> counted loops.
>
> Also I don't see why SCEV needs to be able to analyze the IV to check
> for validity.
>
> Can you please split the patch into the part I suggested (which is ok)
> and the rest?
>
> Thanks,
> Richard.
>
>>
>> Bootstrapping and regression testing did not show any new failures.
>>
>> Is it OK for trunk.
>>
>> ChangeLog:
>> 2014-12-16  Yuri Rumyantsev  <ysrumyan@gmail.com>
>>
>> PR tree-optimization/68021
>> PR tree-optimization/68906
>> * tree-ssa-loop-unswitch.c : Include couple header files.
>> (tree_unswitch_outer_loop): Add check that an exit is not inside inner
>> loop, use number_of_latch_executions to detect non-iterated loops.
>>
>> gcc/testsuite/ChangeLog
>> * gcc.dg/torture/pr68021.c: New test.
>> * gcc.dg/torture/pr68906.c: Likewise.
>>
>> 2015-12-16 15:51 GMT+03:00 Richard Biener <richard.guenther@gmail.com>:
>>> On Wed, Dec 16, 2015 at 1:14 PM, Yuri Rumyantsev <ysrumyan@gmail.com> wrote:
>>>> Hi All,
>>>>
>>>> Here is simple patch which cures the issue with outer-loop unswitching
>>>> - added invocation of number_of_latch_executions() to reject
>>>> unswitching for non-iterated loops.
>>>>
>>>> Bootstrapping and regression testing did not show any new failures.
>>>> Is it OK for trunk?
>>>
>>> No, that looks like just papering over the issue.
>>>
>>> The issue (with the 2nd testcase at least) is that single_exit () accepts
>>> an exit from the inner loop.
>>>
>>> Index: gcc/tree-ssa-loop-unswitch.c
>>> ===================================================================
>>> --- gcc/tree-ssa-loop-unswitch.c        (revision 231686)
>>> +++ gcc/tree-ssa-loop-unswitch.c        (working copy)
>>> @@ -431,7 +431,7 @@ tree_unswitch_outer_loop (struct loop *l
>>>      return false;
>>>    /* Accept loops with single exit only.  */
>>>    exit = single_exit (loop);
>>> -  if (!exit)
>>> +  if (!exit || exit->src->loop_father != loop)
>>>      return false;
>>>    /* Check that phi argument of exit edge is not defined inside loop.  */
>>>    if (!check_exit_phi (loop))
>>>
>>> fixes the runtime testcase for me (not suitable for the testsuite due
>>> to the infinite
>>> looping though).
>>>
>>> Can you please bootstrap/test the above with your testcase?  The above patch is
>>> ok if it passes testing (no time myself right now)
>>>
>>> Thanks,
>>> Richard.
>>>
>>>> ChangeLog:
>>>>
>>>> 2014-12-16  Yuri Rumyantsev  <ysrumyan@gmail.com>
>>>>
>>>> PR tree-optimization/68906
>>>> * tree-ssa-loop-unswitch.c : Include couple header files.
>>>> (tree_unswitch_outer_loop): Use number_of_latch_executions
>>>> to reject non-iterated loops.
>>>>
>>>> gcc/testsuite/ChangeLog
>>>> * gcc.dg/torture/pr68906.c: New test.
Richard Biener Dec. 17, 2015, 12:03 p.m. UTC | #4
On Thu, Dec 17, 2015 at 10:21 AM, Yuri Rumyantsev <ysrumyan@gmail.com> wrote:
> Richard,
>
> Here is modified patch which checks only that exit block belongs to loop.
>
> Bootstrapping and regression testing were successful.
> Is it OK for trunk?

Ok with the spurious

@@ -441,7 +441,7 @@
   iterations = estimated_loop_iterations_int (loop);
   if (iterations >= 0 && iterations <= 1)
     {
-      if (dump_file && (dump_flags & TDF_DETAILS))
+      if (dump_enabled_p ())
        fprintf (dump_file, ";; Not unswitching, loop is not expected"
                 " to iterate\n");
       return false;

hunk removed.

Thanks,
Richard.

> ChangeLog:
> 2014-12-17  Yuri Rumyantsev  <ysrumyan@gmail.com>
>
> PR tree-optimization/68906
> * tree-ssa-loop-unswitch.c (tree_unswitch_outer_loop): Add  a check
> that an exit block belongs to LOOP.
>
> gcc/testsuite/ChangeLog
> * gcc.dg/torture/pr68906.c: New test.
> .
>
> 2015-12-16 17:49 GMT+03:00 Richard Biener <richard.guenther@gmail.com>:
>> On Wed, Dec 16, 2015 at 3:36 PM, Yuri Rumyantsev <ysrumyan@gmail.com> wrote:
>>> Richard,
>>>
>>> Here is updated patch which includes (1) a test on exit proposed by
>>> you and (2) another test from PR68021 which is caught by new check on
>>> counted loop. Outer-loop unswitching is not performed for both new
>>> tests.
>>
>> As said I don't think
>>
>>    /* If the loop is not expected to iterate, there is no need
>>        for unswitching.  */
>> -  iterations = estimated_loop_iterations_int (loop);
>> -  if (iterations >= 0 && iterations <= 1)
>> +  niters = number_of_latch_executions (loop);
>> +  if (!niters || chrec_contains_undetermined (niters))
>>      {
>>
>> is good.  We do want to retain the estimated_loop_iterations check
>> as it takes into account profile data while yours lets through all
>> counted loops.
>>
>> Also I don't see why SCEV needs to be able to analyze the IV to check
>> for validity.
>>
>> Can you please split the patch into the part I suggested (which is ok)
>> and the rest?
>>
>> Thanks,
>> Richard.
>>
>>>
>>> Bootstrapping and regression testing did not show any new failures.
>>>
>>> Is it OK for trunk.
>>>
>>> ChangeLog:
>>> 2014-12-16  Yuri Rumyantsev  <ysrumyan@gmail.com>
>>>
>>> PR tree-optimization/68021
>>> PR tree-optimization/68906
>>> * tree-ssa-loop-unswitch.c : Include couple header files.
>>> (tree_unswitch_outer_loop): Add check that an exit is not inside inner
>>> loop, use number_of_latch_executions to detect non-iterated loops.
>>>
>>> gcc/testsuite/ChangeLog
>>> * gcc.dg/torture/pr68021.c: New test.
>>> * gcc.dg/torture/pr68906.c: Likewise.
>>>
>>> 2015-12-16 15:51 GMT+03:00 Richard Biener <richard.guenther@gmail.com>:
>>>> On Wed, Dec 16, 2015 at 1:14 PM, Yuri Rumyantsev <ysrumyan@gmail.com> wrote:
>>>>> Hi All,
>>>>>
>>>>> Here is simple patch which cures the issue with outer-loop unswitching
>>>>> - added invocation of number_of_latch_executions() to reject
>>>>> unswitching for non-iterated loops.
>>>>>
>>>>> Bootstrapping and regression testing did not show any new failures.
>>>>> Is it OK for trunk?
>>>>
>>>> No, that looks like just papering over the issue.
>>>>
>>>> The issue (with the 2nd testcase at least) is that single_exit () accepts
>>>> an exit from the inner loop.
>>>>
>>>> Index: gcc/tree-ssa-loop-unswitch.c
>>>> ===================================================================
>>>> --- gcc/tree-ssa-loop-unswitch.c        (revision 231686)
>>>> +++ gcc/tree-ssa-loop-unswitch.c        (working copy)
>>>> @@ -431,7 +431,7 @@ tree_unswitch_outer_loop (struct loop *l
>>>>      return false;
>>>>    /* Accept loops with single exit only.  */
>>>>    exit = single_exit (loop);
>>>> -  if (!exit)
>>>> +  if (!exit || exit->src->loop_father != loop)
>>>>      return false;
>>>>    /* Check that phi argument of exit edge is not defined inside loop.  */
>>>>    if (!check_exit_phi (loop))
>>>>
>>>> fixes the runtime testcase for me (not suitable for the testsuite due
>>>> to the infinite
>>>> looping though).
>>>>
>>>> Can you please bootstrap/test the above with your testcase?  The above patch is
>>>> ok if it passes testing (no time myself right now)
>>>>
>>>> Thanks,
>>>> Richard.
>>>>
>>>>> ChangeLog:
>>>>>
>>>>> 2014-12-16  Yuri Rumyantsev  <ysrumyan@gmail.com>
>>>>>
>>>>> PR tree-optimization/68906
>>>>> * tree-ssa-loop-unswitch.c : Include couple header files.
>>>>> (tree_unswitch_outer_loop): Use number_of_latch_executions
>>>>> to reject non-iterated loops.
>>>>>
>>>>> gcc/testsuite/ChangeLog
>>>>> * gcc.dg/torture/pr68906.c: New test.

Patch
diff mbox

Index: gcc/tree-ssa-loop-unswitch.c
===================================================================
--- gcc/tree-ssa-loop-unswitch.c        (revision 231686)
+++ gcc/tree-ssa-loop-unswitch.c        (working copy)
@@ -431,7 +431,7 @@  tree_unswitch_outer_loop (struct loop *l
     return false;
   /* Accept loops with single exit only.  */
   exit = single_exit (loop);
-  if (!exit)
+  if (!exit || exit->src->loop_father != loop)
     return false;
   /* Check that phi argument of exit edge is not defined inside loop.  */
   if (!check_exit_phi (loop))