Patchwork Fix pr57637

login
register
mail settings
Submitter Zhenqiang Chen
Date June 27, 2013, 9:02 a.m.
Message ID <CACgzC7Cab2ke3L0AqGUvqdxg-PEf2r-xkwHq=OYu4UviEhyw6g@mail.gmail.com>
Download mbox | patch
Permalink /patch/255018/
State New
Headers show

Comments

Zhenqiang Chen - June 27, 2013, 9:02 a.m.
Hi,

Shrink-wrap optimization sinks some instructions for more
opportunities. It uses DF_LR_BB_INFO (bb)->def to check whether BB
clobbers SRC. But for ARM, gcc might generate cond_exec insns before
shrink-wrapping. And DF_LR_BB_INFO (bb)->def does not include def info
from cond_exec insns. So the check in function
move_insn_for_shrink_wrap is not enough.

The patch is to add more check bases on DF_LIVE_BB_INFO (bb)->gen if
df-live is available.

Bootstrap and no make check regression on x86-64 and Panda board.

Is is OK for trunk?

Thanks!
-Zhenqiang

ChangeLog:
2013-06-27  Zhenqiang Chen  <zhenqiang.chen@linaro.org>

        PR target/57637
        * function.c (move_insn_for_shrink_wrap): Check
DF_LIVE_BB_INFO (bb)->gen.
Richard Earnshaw - June 27, 2013, 1:10 p.m.
On 27/06/13 10:02, Zhenqiang Chen wrote:
> Hi,
>
> Shrink-wrap optimization sinks some instructions for more
> opportunities. It uses DF_LR_BB_INFO (bb)->def to check whether BB
> clobbers SRC. But for ARM, gcc might generate cond_exec insns before
> shrink-wrapping. And DF_LR_BB_INFO (bb)->def does not include def info
> from cond_exec insns. So the check in function
> move_insn_for_shrink_wrap is not enough.
>
> The patch is to add more check bases on DF_LIVE_BB_INFO (bb)->gen if
> df-live is available.
>
> Bootstrap and no make check regression on x86-64 and Panda board.
>
> Is is OK for trunk?
>
> Thanks!
> -Zhenqiang
>
> ChangeLog:
> 2013-06-27  Zhenqiang Chen  <zhenqiang.chen@linaro.org>
>
>          PR target/57637
>          * function.c (move_insn_for_shrink_wrap): Check
> DF_LIVE_BB_INFO (bb)->gen.

First off, this isn't really my area, so all this might be off base. 
Did you really mean Richard Sandiford?

The code you're looking at here looks a bit odd.  We have

       live_out = df_get_live_out (bb);
       live_in = df_get_live_in (next_block);
       bb = next_block;

       /* Check whether BB uses DEST or clobbers DEST.  We need to add
	 INSN to BB if so.  Either way, DEST is no longer live on entry,
	 except for any part that overlaps SRC (next loop).  */
       bb_uses = &DF_LR_BB_INFO (bb)->use;
       bb_defs = &DF_LR_BB_INFO (bb)->def;

The setting of live_out and live in uses

   if (df_live)
     return DF_LIVE_OUT (bb);
   else
     return DF_LR_OUT (bb);

but for bb_uses and bb_defs, we unconditionally use the LR problem and 
never consider live.

That seems strange to me.

Perhaps a better fix would be to set bb_uses and bb_defs based on 
whether or not df_live was valid.  ie

       live_out = df_get_live_out (bb);
       live_in = df_get_live_in (next_block);
       bb = next_block;

       /* Check whether BB uses DEST or clobbers DEST.  We need to add
	 INSN to BB if so.  Either way, DEST is no longer live on entry,
	 except for any part that overlaps SRC (next loop).  */
       if (df_live)
         {
           bb_uses = &DF_LIVE_BB_INFO (bb)->use;
           bb_defs = &DF_LIVE_BB_INFO (bb)->def;
         }
       else
         {
           bb_uses = &DF_LR_BB_INFO (bb)->use;
           bb_defs = &DF_LR_BB_INFO (bb)->def;
         }

Richard (S), what are your thoughts?

R.

>
> diff --git a/gcc/function.c b/gcc/function.c
> index 3e33fc7..08ca4a1 100644
> --- a/gcc/function.c
> +++ b/gcc/function.c
> @@ -5508,7 +5508,8 @@ move_insn_for_shrink_wrap (basic_block bb, rtx insn,
>         bb_defs = &DF_LR_BB_INFO (bb)->def;
>         for (i = dregno; i < end_dregno; i++)
>          {
> -         if (REGNO_REG_SET_P (bb_uses, i) || REGNO_REG_SET_P (bb_defs, i))
> +         if (REGNO_REG_SET_P (bb_uses, i) || REGNO_REG_SET_P (bb_defs, i)
> +             || (df_live && REGNO_REG_SET_P (&DF_LIVE_BB_INFO (bb)->gen, i)))
>              next_block = NULL;
>            CLEAR_REGNO_REG_SET (live_out, i);
>            CLEAR_REGNO_REG_SET (live_in, i);
> @@ -5518,7 +5519,8 @@ move_insn_for_shrink_wrap (basic_block bb, rtx insn,
>           Either way, SRC is now live on entry.  */
>         for (i = sregno; i < end_sregno; i++)
>          {
> -         if (REGNO_REG_SET_P (bb_defs, i))
> +         if (REGNO_REG_SET_P (bb_defs, i)
> +             || (df_live && REGNO_REG_SET_P (&DF_LIVE_BB_INFO (bb)->gen, i)))
>              next_block = NULL;
>            SET_REGNO_REG_SET (live_out, i);
>            SET_REGNO_REG_SET (live_in, i);
>
Zhenqiang Chen - June 28, 2013, 7:02 a.m.
On 27 June 2013 21:10, Richard Earnshaw <rearnsha@arm.com> wrote:
> On 27/06/13 10:02, Zhenqiang Chen wrote:
>>
>> Hi,
>>
>> Shrink-wrap optimization sinks some instructions for more
>> opportunities. It uses DF_LR_BB_INFO (bb)->def to check whether BB
>> clobbers SRC. But for ARM, gcc might generate cond_exec insns before
>> shrink-wrapping. And DF_LR_BB_INFO (bb)->def does not include def info
>> from cond_exec insns. So the check in function
>> move_insn_for_shrink_wrap is not enough.
>>
>> The patch is to add more check bases on DF_LIVE_BB_INFO (bb)->gen if
>> df-live is available.
>>
>> Bootstrap and no make check regression on x86-64 and Panda board.
>>
>> Is is OK for trunk?
>>
>> Thanks!
>> -Zhenqiang
>>
>> ChangeLog:
>> 2013-06-27  Zhenqiang Chen  <zhenqiang.chen@linaro.org>
>>
>>          PR target/57637
>>          * function.c (move_insn_for_shrink_wrap): Check
>> DF_LIVE_BB_INFO (bb)->gen.
>
>
> First off, this isn't really my area, so all this might be off base. Did you
> really mean Richard Sandiford?
>
> The code you're looking at here looks a bit odd.  We have
>
>       live_out = df_get_live_out (bb);
>       live_in = df_get_live_in (next_block);
>       bb = next_block;
>
>       /* Check whether BB uses DEST or clobbers DEST.  We need to add
>          INSN to BB if so.  Either way, DEST is no longer live on entry,
>          except for any part that overlaps SRC (next loop).  */
>       bb_uses = &DF_LR_BB_INFO (bb)->use;
>
>       bb_defs = &DF_LR_BB_INFO (bb)->def;
>
> The setting of live_out and live in uses
>
>   if (df_live)
>     return DF_LIVE_OUT (bb);
>   else
>     return DF_LR_OUT (bb);
>
> but for bb_uses and bb_defs, we unconditionally use the LR problem and never
> consider live.
>
> That seems strange to me.
>
> Perhaps a better fix would be to set bb_uses and bb_defs based on whether or
> not df_live was valid.  ie
>
>       live_out = df_get_live_out (bb);
>       live_in = df_get_live_in (next_block);
>       bb = next_block;
>
>       /* Check whether BB uses DEST or clobbers DEST.  We need to add
>          INSN to BB if so.  Either way, DEST is no longer live on entry,
>          except for any part that overlaps SRC (next loop).  */
>       if (df_live)
>         {
>           bb_uses = &DF_LIVE_BB_INFO (bb)->use;
>           bb_defs = &DF_LIVE_BB_INFO (bb)->def;
>         }
>       else
>         {
>           bb_uses = &DF_LR_BB_INFO (bb)->use;
>
>           bb_defs = &DF_LR_BB_INFO (bb)->def;
>         }

Thanks for the comments.

DF_LIVE_BB_INFO includes "gen" and "kill", not "def" and "use".  "gen"
from df_live does not cover all the information of "def" from df_lr. I
had tried to set bb_defs to &DF_LIVE_BB_INFO (bb)->gen. But x86-64
bootstrap failed.

-Zhenqiang

>>
>> diff --git a/gcc/function.c b/gcc/function.c
>> index 3e33fc7..08ca4a1 100644
>> --- a/gcc/function.c
>> +++ b/gcc/function.c
>> @@ -5508,7 +5508,8 @@ move_insn_for_shrink_wrap (basic_block bb, rtx insn,
>>         bb_defs = &DF_LR_BB_INFO (bb)->def;
>>         for (i = dregno; i < end_dregno; i++)
>>          {
>> -         if (REGNO_REG_SET_P (bb_uses, i) || REGNO_REG_SET_P (bb_defs,
>> i))
>> +         if (REGNO_REG_SET_P (bb_uses, i) || REGNO_REG_SET_P (bb_defs, i)
>> +             || (df_live && REGNO_REG_SET_P (&DF_LIVE_BB_INFO (bb)->gen,
>> i)))
>>              next_block = NULL;
>>            CLEAR_REGNO_REG_SET (live_out, i);
>>            CLEAR_REGNO_REG_SET (live_in, i);
>> @@ -5518,7 +5519,8 @@ move_insn_for_shrink_wrap (basic_block bb, rtx insn,
>>           Either way, SRC is now live on entry.  */
>>         for (i = sregno; i < end_sregno; i++)
>>          {
>> -         if (REGNO_REG_SET_P (bb_defs, i))
>> +         if (REGNO_REG_SET_P (bb_defs, i)
>> +             || (df_live && REGNO_REG_SET_P (&DF_LIVE_BB_INFO (bb)->gen,
>> i)))
>>              next_block = NULL;
>>            SET_REGNO_REG_SET (live_out, i);
>>            SET_REGNO_REG_SET (live_in, i);
>>
>
>
Eric Botcazou - July 11, 2013, 10:31 a.m.
> Shrink-wrap optimization sinks some instructions for more
> opportunities. It uses DF_LR_BB_INFO (bb)->def to check whether BB
> clobbers SRC. But for ARM, gcc might generate cond_exec insns before
> shrink-wrapping. And DF_LR_BB_INFO (bb)->def does not include def info
> from cond_exec insns. So the check in function
> move_insn_for_shrink_wrap is not enough.

Posting a testcase would be even better, but the analysis looks plausible.
DF_REF_CONDITIONAL and DF_REF_PARTIAL defs aren't killing defs so they are not 
included in the def set for LR.

As you pointed out, they are included in the gen set for LIVE if it exists, so 
I guess we should wonder if we really want to do the scanning if LIVE doesn't 
exist, i.e. at -O1, instead of simply giving up.  In any case, the scanning 
doesn't need to be redundant with the previous work, so:

      if (!(DF_REF_FLAGS (def) & (DF_REF_PARTIAL | DF_REF_CONDITIONAL)))
	  continue;

is missing in the patch.
Zhenqiang Chen - July 16, 2013, 9:29 a.m.
On 11 July 2013 18:31, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Shrink-wrap optimization sinks some instructions for more
>> opportunities. It uses DF_LR_BB_INFO (bb)->def to check whether BB
>> clobbers SRC. But for ARM, gcc might generate cond_exec insns before
>> shrink-wrapping. And DF_LR_BB_INFO (bb)->def does not include def info
>> from cond_exec insns. So the check in function
>> move_insn_for_shrink_wrap is not enough.
>
> Posting a testcase would be even better, but the analysis looks plausible.
> DF_REF_CONDITIONAL and DF_REF_PARTIAL defs aren't killing defs so they are not
> included in the def set for LR.
>
> As you pointed out, they are included in the gen set for LIVE if it exists, so
> I guess we should wonder if we really want to do the scanning if LIVE doesn't
> exist, i.e. at -O1, instead of simply giving up.  In any case, the scanning

Thanks.

The patch is updated based the comments: it will check GEN set of LIVE
if df_live exists. Otherwise, just give up.

And a testcase is included in the attached patch.

-Zhenqiang


> doesn't need to be redundant with the previous work, so:
>
>       if (!(DF_REF_FLAGS (def) & (DF_REF_PARTIAL | DF_REF_CONDITIONAL)))
>           continue;
>
> is missing in the patch.
>
> --
> Eric Botcazou
Zhenqiang Chen - July 22, 2013, 5:48 a.m.
Ping?

Is the updated patch OK for trunk?

Thanks!
-Zhenqiang

On 16 July 2013 17:29, Zhenqiang Chen <zhenqiang.chen@linaro.org> wrote:
> On 11 July 2013 18:31, Eric Botcazou <ebotcazou@adacore.com> wrote:
>>> Shrink-wrap optimization sinks some instructions for more
>>> opportunities. It uses DF_LR_BB_INFO (bb)->def to check whether BB
>>> clobbers SRC. But for ARM, gcc might generate cond_exec insns before
>>> shrink-wrapping. And DF_LR_BB_INFO (bb)->def does not include def info
>>> from cond_exec insns. So the check in function
>>> move_insn_for_shrink_wrap is not enough.
>>
>> Posting a testcase would be even better, but the analysis looks plausible.
>> DF_REF_CONDITIONAL and DF_REF_PARTIAL defs aren't killing defs so they are not
>> included in the def set for LR.
>>
>> As you pointed out, they are included in the gen set for LIVE if it exists, so
>> I guess we should wonder if we really want to do the scanning if LIVE doesn't
>> exist, i.e. at -O1, instead of simply giving up.  In any case, the scanning
>
> Thanks.
>
> The patch is updated based the comments: it will check GEN set of LIVE
> if df_live exists. Otherwise, just give up.
>
> And a testcase is included in the attached patch.
>
> -Zhenqiang
>
>
>> doesn't need to be redundant with the previous work, so:
>>
>>       if (!(DF_REF_FLAGS (def) & (DF_REF_PARTIAL | DF_REF_CONDITIONAL)))
>>           continue;
>>
>> is missing in the patch.
>>
>> --
>> Eric Botcazou
Eric Botcazou - July 22, 2013, 9:56 a.m.
> The patch is updated based the comments: it will check GEN set of LIVE
> if df_live exists. Otherwise, just give up.

The patch is missing a ChangeLog.  Otherwise it looks good, modulo:

+	  /* DF_LR_BB_INFO (bb)->def does not kill the DF_REF_PARTIAL and
+	     DF_REF_CONDITIONAL defs.  So if DF_LIVE doesn't exist, i.e.

"does not comprise"

+ at -O1, just give up to search NEXT_BLOCK

"just give up searching"

I'm not sure this matters in practice, but don't we need to set the appropriate 
bit in the GEN set of BB at the end of the function if df_live is non-zero, at 
least for the sake of consistency?

> And a testcase is included in the attached patch.

Thanks.  We generally try to make the testcases self-contained, i.e. remove the 
#include's as much as possible.  It seems that stdlib.h is included for abort; 
if so, remove it and use __builtin_abort instead.  Would it be possible to 
remove stdio.h as well?
Zhenqiang Chen - July 22, 2013, 10:26 a.m.
On 22 July 2013 17:56, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> The patch is updated based the comments: it will check GEN set of LIVE
>> if df_live exists. Otherwise, just give up.
>
> The patch is missing a ChangeLog.  Otherwise it looks good, modulo:
>
> +         /* DF_LR_BB_INFO (bb)->def does not kill the DF_REF_PARTIAL and
> +            DF_REF_CONDITIONAL defs.  So if DF_LIVE doesn't exist, i.e.
>
> "does not comprise"
>
> + at -O1, just give up to search NEXT_BLOCK
>
> "just give up searching"

Updated.

> I'm not sure this matters in practice, but don't we need to set the appropriate
> bit in the GEN set of BB at the end of the function if df_live is non-zero, at
> least for the sake of consistency?

And if df_live is non-zero, do we need update df_lr's IN and OUT? I think we
need another patch to make all these consistency.

>> And a testcase is included in the attached patch.
>
> Thanks.  We generally try to make the testcases self-contained, i.e. remove the
> #include's as much as possible.  It seems that stdlib.h is included for abort;
> if so, remove it and use __builtin_abort instead.  Would it be possible to
> remove stdio.h as well?

Update abort to __builtin_abort and malloc to __builtin_malloc. And
remove all the include.

ChangeLog
2013-07-22  Zhenqiang Chen  <zhenqiang.chen@linaro.org>

        * function.c (move_insn_for_shrink_wrap): check gen of df_live if it
        exists, otherwise (-O1) give up searching.

gcc/testsuite/ChangeLog
2013-07-22  Zhenqiang Chen  <zhenqiang.chen@linaro.org>

        * gcc.target/arm/pr57637.c: New added.

Is it OK?

Thanks!
-Zhenqiang
Eric Botcazou - July 22, 2013, 1:16 p.m.
> And if df_live is non-zero, do we need update df_lr's IN and OUT? I think
> we need another patch to make all these consistency.

Possibly, but this would belong to another patch.  I nevertheless think that we 
should set the bit in the GEN set because we'll be testing the GEN set now.

The patch is OK with this change if it passes the usual testing.

> ChangeLog
> 2013-07-22  Zhenqiang Chen  <zhenqiang.chen@linaro.org>
> 
>         * function.c (move_insn_for_shrink_wrap): check gen of df_live if
> it exists, otherwise (-O1) give up searching.

Capital letter at the beginning, and I'd expand a little on it, something like:

        * function.c (move_insn_for_shrink_wrap): Also check the GEN set of the
	LIVE problem for the liveness analysis if it exists, otherwise give up.

> gcc/testsuite/ChangeLog
> 2013-07-22  Zhenqiang Chen  <zhenqiang.chen@linaro.org>
> 
>         * gcc.target/arm/pr57637.c: New added.

"New testcase"
Zhenqiang Chen - July 30, 2013, 7 a.m.
On 22 July 2013 21:16, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> And if df_live is non-zero, do we need update df_lr's IN and OUT? I think
>> we need another patch to make all these consistency.
>
> Possibly, but this would belong to another patch.  I nevertheless think that we
> should set the bit in the GEN set because we'll be testing the GEN set now.
>
> The patch is OK with this change if it passes the usual testing.

Thanks.

Bootstrap on x86-64, ARM chrome book and Pandaboard.
No make check regression on x86-64 and Pandaboard.

The patch is committed with the ChangeLog updated according to your comments.

-Zhenqiang


>> ChangeLog
>> 2013-07-22  Zhenqiang Chen  <zhenqiang.chen@linaro.org>
>>
>>         * function.c (move_insn_for_shrink_wrap): check gen of df_live if
>> it exists, otherwise (-O1) give up searching.
>
> Capital letter at the beginning, and I'd expand a little on it, something like:
>
>         * function.c (move_insn_for_shrink_wrap): Also check the GEN set of the
>         LIVE problem for the liveness analysis if it exists, otherwise give up.
>
>> gcc/testsuite/ChangeLog
>> 2013-07-22  Zhenqiang Chen  <zhenqiang.chen@linaro.org>
>>
>>         * gcc.target/arm/pr57637.c: New added.
>
> "New testcase"
>
> --
> Eric Botcazou

Patch

diff --git a/gcc/function.c b/gcc/function.c
index 3e33fc7..08ca4a1 100644
--- a/gcc/function.c
+++ b/gcc/function.c
@@ -5508,7 +5508,8 @@  move_insn_for_shrink_wrap (basic_block bb, rtx insn,
       bb_defs = &DF_LR_BB_INFO (bb)->def;
       for (i = dregno; i < end_dregno; i++)
        {
-         if (REGNO_REG_SET_P (bb_uses, i) || REGNO_REG_SET_P (bb_defs, i))
+         if (REGNO_REG_SET_P (bb_uses, i) || REGNO_REG_SET_P (bb_defs, i)
+             || (df_live && REGNO_REG_SET_P (&DF_LIVE_BB_INFO (bb)->gen, i)))
            next_block = NULL;
          CLEAR_REGNO_REG_SET (live_out, i);
          CLEAR_REGNO_REG_SET (live_in, i);
@@ -5518,7 +5519,8 @@  move_insn_for_shrink_wrap (basic_block bb, rtx insn,
         Either way, SRC is now live on entry.  */
       for (i = sregno; i < end_sregno; i++)
        {
-         if (REGNO_REG_SET_P (bb_defs, i))
+         if (REGNO_REG_SET_P (bb_defs, i)
+             || (df_live && REGNO_REG_SET_P (&DF_LIVE_BB_INFO (bb)->gen, i)))
            next_block = NULL;
          SET_REGNO_REG_SET (live_out, i);
          SET_REGNO_REG_SET (live_in, i);