diff mbox

fwprop fix for PR79405

Message ID CAFiYyc2yqnnhe=36rqQ5pbKWUJZKkJp7TDCjH84BABWbYf4KRA@mail.gmail.com
State New
Headers show

Commit Message

Richard Biener Feb. 17, 2017, 9:11 a.m. UTC 
On Fri, Feb 17, 2017 at 10:07 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Thu, Feb 16, 2017 at 8:41 PM, Bernd Schmidt <bschmidt@redhat.com> wrote:
>> We have two registers being assigned to each other:
>>
>>  (set (reg 213) (reg 209))
>>  (set (reg 209) (reg 213))
>>
>> These being the only definitions, we are happy to forward propagate reg 209
>> for reg 213 into a third insn, making a new use for reg 209. We are then
>> happy to forward propagate reg 213 for it in the same insn... ending up in
>> an infinite loop.
>>
>> I don't really see an elegant way to prevent this, so the following just
>> tries to detect the situation (and more general ones) by brute force.
>> Bootstrapped and tested on x86_64-linux, verified that the test passes with
>> a ppc cross, ok?
>
> But isn't the issue that we are walking "all uses" (in random order) rather than
> only processing each stmt once?  That is,
>
>   /* Go through all the uses.  df_uses_create will create new ones at the
>      end, and we'll go through them as well.
>
>      Do not forward propagate addresses into loops until after unrolling.
>      CSE did so because it was able to fix its own mess, but we are not.  */
>
>   for (i = 0; i < DF_USES_TABLE_SIZE (); i++)
>     {
>       df_ref use = DF_USES_GET (i);
>       if (use)
>         if (DF_REF_TYPE (use) == DF_REF_REG_USE
>             || DF_REF_BB (use)->loop_father == NULL
>             /* The outer most loop is not really a loop.  */
>             || loop_outer (DF_REF_BB (use)->loop_father) == NULL)
>           forward_propagate_into (use);
>     }
>
> if that were simply walking all instructions, doing forward_propagat_into on
> each use on an instruction we'd avoid the cycle (because we stop propagating).
>
> Because when propagating DF_USES_TABLE changes.

Which either means we might even miss visiting some uses or a fix as simple as


might work?  (not knowing too much about this detail of the DF data
structures - can
the table shrink?)

Richard.

> Richard.
>
>
>>
>>
>> Bernd
>>

Comments

Bernd Schmidt Feb. 20, 2017, 1:58 p.m. UTC | #1 
On 02/17/2017 10:11 AM, Richard Biener wrote:
> Index: gcc/fwprop.c
> ===================================================================
> --- gcc/fwprop.c        (revision 245501)
> +++ gcc/fwprop.c        (working copy)
> @@ -1478,7 +1478,8 @@ fwprop (void)
>       Do not forward propagate addresses into loops until after unrolling.
>       CSE did so because it was able to fix its own mess, but we are not.  */
>
> -  for (i = 0; i < DF_USES_TABLE_SIZE (); i++)
> +  unsigned sz = DF_USES_TABLE_SIZE ();
> +  for (i = 0; i < sz; i++)
>      {
>        df_ref use = DF_USES_GET (i);
>        if (use)
>
> might work?  (not knowing too much about this detail of the DF data
> structures - can the table shrink?)

This would probably work to fix the bug, but this behaviour is 
explicitly documented as intentional (in the comment the second half of 
which you've quoted). I assume it enables additional substitutions.


Bernd
Richard Biener Feb. 20, 2017, 5:13 p.m. UTC | #2 
On February 20, 2017 2:58:54 PM GMT+01:00, Bernd Schmidt <bschmidt@redhat.com> wrote:
>On 02/17/2017 10:11 AM, Richard Biener wrote:
>> Index: gcc/fwprop.c
>> ===================================================================
>> --- gcc/fwprop.c        (revision 245501)
>> +++ gcc/fwprop.c        (working copy)
>> @@ -1478,7 +1478,8 @@ fwprop (void)
>>       Do not forward propagate addresses into loops until after
>unrolling.
>>       CSE did so because it was able to fix its own mess, but we are
>not.  */
>>
>> -  for (i = 0; i < DF_USES_TABLE_SIZE (); i++)
>> +  unsigned sz = DF_USES_TABLE_SIZE ();
>> +  for (i = 0; i < sz; i++)
>>      {
>>        df_ref use = DF_USES_GET (i);
>>        if (use)
>>
>> might work?  (not knowing too much about this detail of the DF data
>> structures - can the table shrink?)
>
>This would probably work to fix the bug, but this behaviour is 
>explicitly documented as intentional (in the comment the second half of
>
>which you've quoted). I assume it enables additional substitutions.

Hmm, this means the walking-stmts solution sounds more correct and also gets these second-level opportunities.

Richard.

>
>Bernd
diff mbox

Patch

Index: gcc/fwprop.c
===================================================================
--- gcc/fwprop.c        (revision 245501)
+++ gcc/fwprop.c        (working copy)
@@ -1478,7 +1478,8 @@  fwprop (void)
      Do not forward propagate addresses into loops until after unrolling.
      CSE did so because it was able to fix its own mess, but we are not.  */

-  for (i = 0; i < DF_USES_TABLE_SIZE (); i++)
+  unsigned sz = DF_USES_TABLE_SIZE ();
+  for (i = 0; i < sz; i++)
     {
       df_ref use = DF_USES_GET (i);
       if (use)