Re: [PATCH] Fix pr57637

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

Hi,

The patch is updated. If there is no df_live, we still can not
guarantee the correctness. So the new patch just checks the
DF_INSN_DEFS.

Bootstrap and no make check regression on x86-64.
Bootstrap on ARM chrome book.

Is it OK?

Thanks!
-Zhenqiang

Changelog:
2013-07-05  Zhenqiang Chen  <zhenqiang.chen@linaro.org>

        PR target/57637
        * function.c (move_insn_for_shrink_wrap): Check DF_INSN_DEFS.

diff --git a/gcc/function.c b/gcc/function.c
index 3e33fc7..28f1b78 100644
--- a/gcc/function.c
+++ b/gcc/function.c
@@ -5524,12 +5524,40 @@ move_insn_for_shrink_wrap (basic_block bb, rtx insn,
 	  SET_REGNO_REG_SET (live_in, i);
 	}

+      /* DF_LR_BB_INFO (bb)->def does not cover the DF_REF_PARTIAL and
+	 DF_REF_CONDITIONAL def.  So recheck the DF_INSN_DEFS.  */
+      if (next_block)
+	{
+	  rtx x;
+	  df_ref *def_rec;
+
+	  FOR_BB_INSNS(bb, x)
+	    {
+	      if (!NONDEBUG_INSN_P (x))
+		continue;
+
+	      for (def_rec = DF_INSN_DEFS (x); *def_rec; def_rec++)
+		{
+		  df_ref def = *def_rec;
+		  unsigned int regno = DF_REF_REGNO (def);
+
+		  for (i = dregno; i < end_dregno; i++)
+		    if (i == regno)
+		      goto DONE;
+		  for (i = sregno; i < end_sregno; i++)
+		    if (i == regno)
+		      goto DONE;
+		}
+	    }
+	}
+
       /* If we don't need to add the move to BB, look for a single
 	 successor block.  */
       if (next_block)
 	next_block = next_block_for_reg (next_block, dregno, end_dregno);
     }
   while (next_block);
+DONE:

   /* BB now defines DEST.  It only uses the parts of DEST that overlap SRC
      (next loop).  */


On 28 June 2013 15:02, Zhenqiang Chen <zhenqiang.chen@linaro.org> wrote:
> 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);
>>>
>>
>>