Re: [PATCH] Fix PR rtl-optimization/61278

[Zhenqiang Chen <zhenqiang dot chen at linaro dot org>]

On 23 May 2014 19:56, Richard Biener <richard.guenther@gmail.com> wrote:
> On Fri, May 23, 2014 at 12:33 PM, Zhenqiang Chen
> <zhenqiang.chen@linaro.org> wrote:
>> On 23 May 2014 17:05, Richard Biener <richard.guenther@gmail.com> wrote:
>>> On Fri, May 23, 2014 at 9:23 AM, Zhenqiang Chen
>>> <zhenqiang.chen@linaro.org> wrote:
>>>> Hi,
>>>>
>>>> The patch fixes PR rtl-optimization/61278. Root cause for issue is
>>>> that df_live does not exist at -O1.
>>>>
>>>> Bootstrap and no make check regression on X86-64.
>>>>
>>>> OK for trunk?
>>>
>>> Why do you need to give up?  It seems you can simply avoid
>>> marking the block as dirty (though df_get_live_in/out also hands you
>>> back DF_LR_IN/OUT if !df_live).  So isn't the df_grow_bb_info the
>>> "real" fix?
>>
>> The df_get_live_in of the new basic block will be used to analyse
>> later INSNs. If it is not set or incorrect, it will impact on later
>> analysis.
>> df_grow_bb_info is to make sure the live_in data structure is
>> allocated for the new basic block (although I have not found any case
>> fail without it). After bitmap_copy(...), we can use it for later
>> INSNs.
>>
>>> Note that df_get_live_in/out are functions tailored to IRA that
>>> knows that they handle both df_live and df_lr dependent on
>>> optimization level.  Is shrink-wrapping supposed to work with
>>> both problems as well?
>>
>> Yes. But it seams not perfect to handle df_lr problem. When I fixed PR
>> 57637 (https://gcc.gnu.org/ml/gcc-patches/2013-07/msg00897.html), we
>> selected "if DF_LIVE doesn't exist, i.e. at -O1, just give up
>> searching NEXT_BLOCK."
>
> Ok, I see.  Maybe it would be better to completely disable
> shrink-wrapping when LIVE is not available.

Agree. There are potential bugs since DF_REF_PARTIAL and
DF_REF_CONDITIONAL defs are not correctly handled in DF_LR.

I will send out a patch to disable prepare_shrink_wrap when DF_LIVE is
not available.

> Patch is ok.

Thanks! Committed @210922.

-Zhenqiang

> Thanks,
> Richard.
>
>> Thanks!
>> -Zhenqiang
>>
>>>>
>>>> ChangeLog:
>>>> 2014-05-23  Zhenqiang Chen  <zhenqiang.chen@linaro.org>
>>>>
>>>>         PR rtl-optimization/61278
>>>>         * shrink-wrap.c (move_insn_for_shrink_wrap): Check df_live.
>>>>
>>>> testsuite/ChangeLog:
>>>> 2014-05-23  Zhenqiang Chen  <zhenqiang.chen@linaro.org>
>>>>
>>>>         * gcc.dg/lto/pr61278_0.c: New test.
>>>>         * gcc.dg/lto/pr61278_1.c: New test.
>>>>
>>>> diff --git a/gcc/shrink-wrap.c b/gcc/shrink-wrap.c
>>>> index f09cfe7..be17829 100644
>>>> --- a/gcc/shrink-wrap.c
>>>> +++ b/gcc/shrink-wrap.c
>>>> @@ -204,8 +204,15 @@ move_insn_for_shrink_wrap (basic_block bb, rtx insn,
>>>>    /* Create a new basic block on the edge.  */
>>>>    if (EDGE_COUNT (next_block->preds) == 2)
>>>>      {
>>>> +      /* If DF_LIVE doesn't exist, i.e. at -O1, just give up.  */
>>>> +      if (!df_live)
>>>> +       return false;
>>>> +
>>>>        next_block = split_edge (live_edge);
>>>>
>>>> +      /* We create a new basic block.  Call df_grow_bb_info to make sure
>>>> +        all data structures are allocated.  */
>>>> +      df_grow_bb_info (df_live);
>>>>        bitmap_copy (df_get_live_in (next_block), df_get_live_out (bb));
>>>>        df_set_bb_dirty (next_block);
>>>>
>>>> diff --git a/gcc/testsuite/gcc.dg/lto/pr61278_0.c
>>>> b/gcc/testsuite/gcc.dg/lto/pr61278_0.c
>>>> new file mode 100644
>>>> index 0000000..03a24ae
>>>> --- /dev/null
>>>> +++ b/gcc/testsuite/gcc.dg/lto/pr61278_0.c
>>>> @@ -0,0 +1,30 @@
>>>> +/* { dg-lto-do link } */
>>>> +/* { dg-lto-options { { -flto -O0 } } } */
>>>> +/* { dg-extra-ld-options " -flto -O1 " } */
>>>> +
>>>> +static unsigned int
>>>> +fn1 (int p1, int p2)
>>>> +{
>>>> +  return 0;
>>>> +}
>>>> +
>>>> +char a, b, c;
>>>> +
>>>> +char
>>>> +foo (char *p)
>>>> +{
>>>> +  int i;
>>>> +  for (b = 1 ; b > 0; b++)
>>>> +    {
>>>> +      for (i = 0; i < 2; i++)
>>>> +       ;
>>>> +      for (a = 1; a > 0; a++)
>>>> +       {
>>>> +         char d[1] = { 0 };
>>>> +         if (*p)
>>>> +           break;
>>>> +         c ^= fn1 (fn1 (fn1 (0, 0), 0), 0);
>>>> +       }
>>>> +    }
>>>> +  return 0;
>>>> +}
>>>> diff --git a/gcc/testsuite/gcc.dg/lto/pr61278_1.c
>>>> b/gcc/testsuite/gcc.dg/lto/pr61278_1.c
>>>> new file mode 100644
>>>> index 0000000..b02c8ac
>>>> --- /dev/null
>>>> +++ b/gcc/testsuite/gcc.dg/lto/pr61278_1.c
>>>> @@ -0,0 +1,13 @@
>>>> +/* { dg-lto-do link } */
>>>> +/* { dg-lto-options { { -flto -O1 } } } */
>>>> +
>>>> +extern char foo (char *);
>>>> +
>>>> +char d;
>>>> +
>>>> +int
>>>> +main ()
>>>> +{
>>>> +  foo (&d);
>>>> +  return 0;
>>>> +}