Patchwork [ARM] Extend thumb1_reorg to save more comparison instructions

login
register
mail settings
Submitter Bin Cheng
Date Sept. 17, 2013, 2:16 a.m.
Message ID <000001ceb34b$ed24bcd0$c76e3670$@arm.com>
Download mbox | patch
Permalink /patch/275354/
State New
Headers show

Comments

Bin Cheng - Sept. 17, 2013, 2:16 a.m.
> -----Original Message-----
> From: Richard Earnshaw
> Sent: Thursday, September 12, 2013 11:24 PM
> To: Bin Cheng
> Cc: gcc-patches@gcc.gnu.org
> Subject: Re: [PATCH ARM]Extend thumb1_reorg to save more comparison
> instructions
> 
> On 18/04/13 06:34, Bin Cheng wrote:
> 
> Sorry for the delay, I've been trying to get my head around this one.
> 
> > thumb1_reorg-20130417.txt
> >
> >
> > Index: gcc/config/arm/arm.c
> >
> ==========================================================
> =========
> > --- gcc/config/arm/arm.c	(revision 197562)
> > +++ gcc/config/arm/arm.c	(working copy)
> > @@ -14026,6 +14026,7 @@ thumb1_reorg (void)
> >        rtx set, dest, src;
> >        rtx pat, op0;
> >        rtx prev, insn = BB_END (bb);
> > +      bool insn_clobbered = false;
> >
> >        while (insn != BB_HEAD (bb) && DEBUG_INSN_P (insn))
> >  	insn = PREV_INSN (insn);
> > @@ -14034,12 +14035,29 @@ thumb1_reorg (void)
> >        if (INSN_CODE (insn) != CODE_FOR_cbranchsi4_insn)
> >  	continue;
> >
> > -      /* Find the first non-note insn before INSN in basic block BB.
*/
> > +      /* Get the register with which we are comparing.  */
> > +      pat = PATTERN (insn);
> > +      op0 = XEXP (XEXP (SET_SRC (pat), 0), 0);
> > +
> > +      /* Find the first flag setting insn before INSN in basic block
> > + BB.  */
> >        gcc_assert (insn != BB_HEAD (bb));
> > -      prev = PREV_INSN (insn);
> > -      while (prev != BB_HEAD (bb) && (NOTE_P (prev) || DEBUG_INSN_P
> (prev)))
> > -	prev = PREV_INSN (prev);
> > +      for (prev = PREV_INSN (insn);
> > +	   (!insn_clobbered
> > +	    && prev != BB_HEAD (bb)
> > +	    && (NOTE_P (prev)
> > +		|| DEBUG_INSN_P (prev)
> > +		|| (GET_CODE (prev) == SET
> 
> This can't be right.  prev is an insn of some form, so the test that it is
a SET will
> always fail.
> 
> What you need to do here is to initialize 'set' to null before the loop
and then
> have something like
> 
> 		|| ((set = single_set (prev)) != NULL
> 
> > +		    && get_attr_conds (prev) == CONDS_NOCOND)));
> > +	   prev = PREV_INSN (prev))
> > +	{
> > +	  if (reg_set_p (op0, prev))
> > +	    insn_clobbered = true;
> > +	}
> >
> > +      /* Skip if op0 is clobbered by insn other than prev. */
> > +      if (insn_clobbered)
> > +	continue;
> > +
> >        set = single_set (prev);
> 
> This now becomes redundant and ...
> 
> >        if (!set)
> >  	continue;
> 
> This will be based on the set you extracted above.
> 

Hi Richard, here is the updated patch according to your comments.  Tested on
thumb1, please review.

Thanks.
bin
Richard Earnshaw - Sept. 17, 2013, 12:51 p.m.
On 17/09/13 03:16, bin.cheng wrote:
> 
> 
>> -----Original Message-----
>> From: Richard Earnshaw
>> Sent: Thursday, September 12, 2013 11:24 PM
>> To: Bin Cheng
>> Cc: gcc-patches@gcc.gnu.org
>> Subject: Re: [PATCH ARM]Extend thumb1_reorg to save more comparison
>> instructions
>>
>> On 18/04/13 06:34, Bin Cheng wrote:
>>
>> Sorry for the delay, I've been trying to get my head around this one.
>>
>>> thumb1_reorg-20130417.txt
>>>
>>>
>>> Index: gcc/config/arm/arm.c
>>>
>> ==========================================================
>> =========
>>> --- gcc/config/arm/arm.c	(revision 197562)
>>> +++ gcc/config/arm/arm.c	(working copy)
>>> @@ -14026,6 +14026,7 @@ thumb1_reorg (void)
>>>        rtx set, dest, src;
>>>        rtx pat, op0;
>>>        rtx prev, insn = BB_END (bb);
>>> +      bool insn_clobbered = false;
>>>
>>>        while (insn != BB_HEAD (bb) && DEBUG_INSN_P (insn))
>>>  	insn = PREV_INSN (insn);
>>> @@ -14034,12 +14035,29 @@ thumb1_reorg (void)
>>>        if (INSN_CODE (insn) != CODE_FOR_cbranchsi4_insn)
>>>  	continue;
>>>
>>> -      /* Find the first non-note insn before INSN in basic block BB.
> */
>>> +      /* Get the register with which we are comparing.  */
>>> +      pat = PATTERN (insn);
>>> +      op0 = XEXP (XEXP (SET_SRC (pat), 0), 0);
>>> +
>>> +      /* Find the first flag setting insn before INSN in basic block
>>> + BB.  */
>>>        gcc_assert (insn != BB_HEAD (bb));
>>> -      prev = PREV_INSN (insn);
>>> -      while (prev != BB_HEAD (bb) && (NOTE_P (prev) || DEBUG_INSN_P
>> (prev)))
>>> -	prev = PREV_INSN (prev);
>>> +      for (prev = PREV_INSN (insn);
>>> +	   (!insn_clobbered
>>> +	    && prev != BB_HEAD (bb)
>>> +	    && (NOTE_P (prev)
>>> +		|| DEBUG_INSN_P (prev)
>>> +		|| (GET_CODE (prev) == SET
>>
>> This can't be right.  prev is an insn of some form, so the test that it is
> a SET will
>> always fail.
>>
>> What you need to do here is to initialize 'set' to null before the loop
> and then
>> have something like
>>
>> 		|| ((set = single_set (prev)) != NULL
>>
>>> +		    && get_attr_conds (prev) == CONDS_NOCOND)));
>>> +	   prev = PREV_INSN (prev))
>>> +	{
>>> +	  if (reg_set_p (op0, prev))
>>> +	    insn_clobbered = true;
>>> +	}
>>>
>>> +      /* Skip if op0 is clobbered by insn other than prev. */
>>> +      if (insn_clobbered)
>>> +	continue;
>>> +
>>>        set = single_set (prev);
>>
>> This now becomes redundant and ...
>>
>>>        if (!set)
>>>  	continue;
>>
>> This will be based on the set you extracted above.
>>
> 
> Hi Richard, here is the updated patch according to your comments.  Tested on
> thumb1, please review.
> 

OK.

R.

Patch

Index: gcc/config/arm/arm.c
===================================================================
--- gcc/config/arm/arm.c	(revision 202599)
+++ gcc/config/arm/arm.c	(working copy)
@@ -14265,9 +14265,10 @@  thumb1_reorg (void)
 
   FOR_EACH_BB (bb)
     {
-      rtx set, dest, src;
-      rtx pat, op0;
+      rtx dest, src;
+      rtx pat, op0, set = NULL;
       rtx prev, insn = BB_END (bb);
+      bool insn_clobbered = false;
 
       while (insn != BB_HEAD (bb) && DEBUG_INSN_P (insn))
 	insn = PREV_INSN (insn);
@@ -14276,13 +14277,29 @@  thumb1_reorg (void)
       if (INSN_CODE (insn) != CODE_FOR_cbranchsi4_insn)
 	continue;
 
-      /* Find the first non-note insn before INSN in basic block BB.  */
+      /* Get the register with which we are comparing.  */
+      pat = PATTERN (insn);
+      op0 = XEXP (XEXP (SET_SRC (pat), 0), 0);
+
+      /* Find the first flag setting insn before INSN in basic block BB.  */
       gcc_assert (insn != BB_HEAD (bb));
-      prev = PREV_INSN (insn);
-      while (prev != BB_HEAD (bb) && (NOTE_P (prev) || DEBUG_INSN_P (prev)))
-	prev = PREV_INSN (prev);
+      for (prev = PREV_INSN (insn);
+	   (!insn_clobbered
+	    && prev != BB_HEAD (bb)
+	    && (NOTE_P (prev)
+		|| DEBUG_INSN_P (prev)
+		|| ((set = single_set (prev)) != NULL
+		    && get_attr_conds (prev) == CONDS_NOCOND)));
+	   prev = PREV_INSN (prev))
+	{
+	  if (reg_set_p (op0, prev))
+	    insn_clobbered = true;
+	}
 
-      set = single_set (prev);
+      /* Skip if op0 is clobbered by insn other than prev. */
+      if (insn_clobbered)
+	continue;
+
       if (!set)
 	continue;
 
@@ -14292,12 +14309,9 @@  thumb1_reorg (void)
 	  || !low_register_operand (src, SImode))
 	continue;
 
-      pat = PATTERN (insn);
-      op0 = XEXP (XEXP (SET_SRC (pat), 0), 0);
       /* Rewrite move into subtract of 0 if its operand is compared with ZERO
-	 in INSN. Don't need to check dest since cprop_hardreg pass propagates
-	 src into INSN.  */
-      if (REGNO (op0) == REGNO (src))
+	 in INSN.  Both src and dest of the move insn are checked.  */
+      if (REGNO (op0) == REGNO (src) || REGNO (op0) == REGNO (dest))
 	{
 	  dest = copy_rtx (dest);
 	  src = copy_rtx (src);