diff mbox

[PING,Updated] : [PATCH GCC/ARM] Fix problem that hardreg_cprop opportunities are missed on thumb1

Message ID 000301cda6cd$ad945b60$08bd1220$@cheng@arm.com
State New
Headers show

Commit Message

Bin Cheng Oct. 10, 2012, 9:57 a.m. UTC 
Ping^2

> -----Original Message-----
> From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-owner@gcc.gnu.org]
On
> Behalf Of Bin Cheng
> Sent: Monday, October 08, 2012 2:36 PM
> To: gcc-patches@gcc.gnu.org
> Cc: Ramana Radhakrishnan; Richard Earnshaw; 'Richard Sandiford'
> Subject: RE: [PING Updated]: [PATCH GCC/ARM] Fix problem that
hardreg_cprop
> opportunities are missed on thumb1
> 
> Ping.
> 
> > -----Original Message-----
> > From: gcc-patches-owner@gcc.gnu.org
> > [mailto:gcc-patches-owner@gcc.gnu.org]
> On
> > Behalf Of Bin Cheng
> > Sent: Tuesday, September 25, 2012 4:00 PM
> > To: 'Richard Sandiford'
> > Cc: Ramana Radhakrishnan; Richard Earnshaw; gcc-patches@gcc.gnu.org
> > Subject: RE: [Updated]: [PATCH GCC/ARM] Fix problem that hardreg_cprop
> > opportunities are missed on thumb1
> >
> >
> > > -----Original Message-----
> > > From: Richard Sandiford [mailto:rdsandiford@googlemail.com]
> > > Sent: Wednesday, September 05, 2012 6:09 AM
> > > To: Bin Cheng
> > > Cc: Ramana Radhakrishnan; 'Eric Botcazou'; gcc-patches@gcc.gnu.org
> > > Subject: Re: Ping: [PATCH GCC/ARM] Fix problem that hardreg_cprop
> > > opportunities are missed on thumb1
> >
> > > Subtraction of zero isn't canonical rtl though.  Passes after
> > > peephole2
> > would
> > > be well within their rights to simplify the expression back to a move.
> > > From that point of view, making the passes recognise (plus X 0) and
> > > (minus
> > X 0)
> > > as special cases would be inconsistent.
> > >
> > > Rather than make the Thumb 1 CC usage implicit in the rtl stream,
> > > and
> > carry
> > > the current state around in cfun->machine, it seems like it would be
> > better to
> > > get md_reorg to rewrite the instructions into a form that makes the
> > > use of condition codes explicit.
> > >
> > > md_reorg also sounds like a better place in the pipeline than
> > > peephole2 to
> > be
> > > doing this kind of transformation, although I admit I have zero
> > > evidence
> > to
> > > back that up...
> > >
> >
> > Hi Richard,
> >
> > This is the updated patch according to your suggestions. I removed the
> > peephole2 patterns and introduced function thumb1_reorg to rewrite
> > instructions in md_reorg pass.
> >
> > In addition to missed propagation, this patch also detects following
case:
> >       mov r5, r0
> >       str r0, [r4]   <-------miscellaneous irrelevant instructions
> >       [cmp r0, 0]    <-------saved
> >       bne  .Lxxx
> >
> > Patch tested on arm-none-eabi/cortex-m0, no regressions introduced.
> >
> > Is it OK?
> >
> > Thanks.
> >
> > 2012-09-25  Bin Cheng  <bin.cheng@arm.com>
> >
> > 	* config/arm/arm.c (thumb1_reorg): New function.
> > 	(arm_reorg): Call thumb1_reorg.
> > 	(thumb1_final_prescan_insn): Record src operand in thumb1_cc_op0.
> > 	* config/arm/arm.md : Remove peephole2 patterns which rewrites move
> > 	into subtract of ZERO.
> 
> 
>

Comments

Bin Cheng Nov. 22, 2012, 1:46 a.m. UTC | #1 
> -----Original Message-----
> From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-owner@gcc.gnu.org]
On
> Behalf Of Bin Cheng
> Sent: Wednesday, October 10, 2012 5:58 PM
> To: gcc-patches@gcc.gnu.org
> Cc: Ramana Radhakrishnan; Richard Earnshaw; 'Richard Sandiford'
> Subject: RE: [PING Updated]: [PATCH GCC/ARM] Fix problem that
hardreg_cprop
> opportunities are missed on thumb1
> 
> Ping^2
> 
> > -----Original Message-----
> > From: gcc-patches-owner@gcc.gnu.org
> > [mailto:gcc-patches-owner@gcc.gnu.org]
> On
> > Behalf Of Bin Cheng
> > Sent: Monday, October 08, 2012 2:36 PM
> > To: gcc-patches@gcc.gnu.org
> > Cc: Ramana Radhakrishnan; Richard Earnshaw; 'Richard Sandiford'
> > Subject: RE: [PING Updated]: [PATCH GCC/ARM] Fix problem that
> hardreg_cprop
> > opportunities are missed on thumb1
> >
> > Ping.
> >
> > > -----Original Message-----
> > > From: gcc-patches-owner@gcc.gnu.org
> > > [mailto:gcc-patches-owner@gcc.gnu.org]
> > On
> > > Behalf Of Bin Cheng
> > > Sent: Tuesday, September 25, 2012 4:00 PM
> > > To: 'Richard Sandiford'
> > > Cc: Ramana Radhakrishnan; Richard Earnshaw; gcc-patches@gcc.gnu.org
> > > Subject: RE: [Updated]: [PATCH GCC/ARM] Fix problem that
> > > hardreg_cprop opportunities are missed on thumb1
> > >
> > >
> > > > -----Original Message-----
> > > > From: Richard Sandiford [mailto:rdsandiford@googlemail.com]
> > > > Sent: Wednesday, September 05, 2012 6:09 AM
> > > > To: Bin Cheng
> > > > Cc: Ramana Radhakrishnan; 'Eric Botcazou'; gcc-patches@gcc.gnu.org
> > > > Subject: Re: Ping: [PATCH GCC/ARM] Fix problem that hardreg_cprop
> > > > opportunities are missed on thumb1
> > >
> > > > Subtraction of zero isn't canonical rtl though.  Passes after
> > > > peephole2
> > > would
> > > > be well within their rights to simplify the expression back to a
move.
> > > > From that point of view, making the passes recognise (plus X 0)
> > > > and (minus
> > > X 0)
> > > > as special cases would be inconsistent.
> > > >
> > > > Rather than make the Thumb 1 CC usage implicit in the rtl stream,
> > > > and
> > > carry
> > > > the current state around in cfun->machine, it seems like it would
> > > > be
> > > better to
> > > > get md_reorg to rewrite the instructions into a form that makes
> > > > the use of condition codes explicit.
> > > >
> > > > md_reorg also sounds like a better place in the pipeline than
> > > > peephole2 to
> > > be
> > > > doing this kind of transformation, although I admit I have zero
> > > > evidence
> > > to
> > > > back that up...
> > > >
> > >
> > > Hi Richard,
> > >
> > > This is the updated patch according to your suggestions. I removed
> > > the
> > > peephole2 patterns and introduced function thumb1_reorg to rewrite
> > > instructions in md_reorg pass.
> > >
> > > In addition to missed propagation, this patch also detects following
> case:
> > >       mov r5, r0
> > >       str r0, [r4]   <-------miscellaneous irrelevant instructions
> > >       [cmp r0, 0]    <-------saved
> > >       bne  .Lxxx
> > >
> > > Patch tested on arm-none-eabi/cortex-m0, no regressions introduced.
> > >
> > > Is it OK?
> > >
> > > Thanks.
> > >
> > > 2012-09-25  Bin Cheng  <bin.cheng@arm.com>
> > >
> > > 	* config/arm/arm.c (thumb1_reorg): New function.
> > > 	(arm_reorg): Call thumb1_reorg.
> > > 	(thumb1_final_prescan_insn): Record src operand in thumb1_cc_op0.
> > > 	* config/arm/arm.md : Remove peephole2 patterns which rewrites move
> > > 	into subtract of ZERO.
> >
> >
> >
Hi, this patch was posted long before, could somebody help me review this?
http://gcc.gnu.org/ml/gcc-patches/2012-10/msg00967.html

Thanks very much.
Jeff Law Nov. 22, 2012, 6:50 a.m. UTC | #2 
On 11/21/2012 06:46 PM, Bin Cheng wrote:
>
>
>> -----Original Message-----
>> From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-owner@gcc.gnu.org]
> On
>> Behalf Of Bin Cheng
>> Sent: Wednesday, October 10, 2012 5:58 PM
>> To: gcc-patches@gcc.gnu.org
>> Cc: Ramana Radhakrishnan; Richard Earnshaw; 'Richard Sandiford'
>> Subject: RE: [PING Updated]: [PATCH GCC/ARM] Fix problem that
> hardreg_cprop
>> opportunities are missed on thumb1
>>
>> Ping^2
One of the ARM port maintainers should own this.

Nick, Richard, Paul or Ramana (though I haven't seen anything from Paul 
in a while).

jeff
diff mbox

Patch

Index: gcc/config/arm/arm.c
===================================================================
--- gcc/config/arm/arm.c	(revision 191088)
+++ gcc/config/arm/arm.c	(working copy)
@@ -13263,6 +13263,62 @@  note_invalid_constants (rtx insn, HOST_WIDE_INT ad
   return;
 }
 
+/* Rewrite move insn into subtract of 0 if the condition codes will
+   be useful in next conditional jump insn.  */
+
+static void
+thumb1_reorg (void)
+{
+  basic_block bb;
+
+  FOR_EACH_BB (bb)
+    {
+      rtx set, dest, src;
+      rtx pat, op0;
+      rtx prev, insn = BB_END (bb);
+
+      while (insn != BB_HEAD (bb) && DEBUG_INSN_P (insn))
+	insn = PREV_INSN (insn);
+
+      /* Find the last cbranchsi4_insn in basic block BB.  */
+      if (INSN_CODE (insn) != CODE_FOR_cbranchsi4_insn)
+	continue;
+
+      /* Find the first non-note 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);
+
+      set = single_set (prev);
+      if (!set)
+	continue;
+
+      dest = SET_DEST (set);
+      src = SET_SRC (set);
+      if (!low_register_operand (dest, SImode)
+	  || !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))
+	{
+	  dest = copy_rtx (dest);
+	  src = copy_rtx (src);
+	  src = gen_rtx_MINUS (SImode, src, const0_rtx);
+	  PATTERN (prev) = gen_rtx_SET (VOIDmode, dest, src);
+	  INSN_CODE (prev) = -1;
+	  /* Set test register in INSN to dest.  */
+	  XEXP (XEXP (SET_SRC (pat), 0), 0) = copy_rtx (dest);
+	  INSN_CODE (insn) = -1;
+	}
+    }
+}
+
 /* Convert instructions to their cc-clobbering variant if possible, since
    that allows us to use smaller encodings.  */
 
@@ -13459,7 +13515,9 @@  arm_reorg (void)
   HOST_WIDE_INT address = 0;
   Mfix * fix;
 
-  if (TARGET_THUMB2)
+  if (TARGET_THUMB1)
+    thumb1_reorg ();
+  else if (TARGET_THUMB2)
     thumb2_reorg ();
 
   /* Ensure all insns that must be split have been split at this point.
@@ -21736,6 +21794,12 @@  thumb1_final_prescan_insn (rtx insn)
 	      if (src1 == const0_rtx)
 		cfun->machine->thumb1_cc_mode = CCmode;
 	    }
+	  else if (REG_P (SET_DEST (set)) && REG_P (SET_SRC (set)))
+	    {
+	      /* Record the src register operand instead of dest because
+		 cprop_hardreg pass propagates src.  */
+	      cfun->machine->thumb1_cc_op0 = SET_SRC (set);
+	    }
 	}
       else if (conds != CONDS_NOCOND)
 	cfun->machine->thumb1_cc_insn = NULL_RTX;
Index: gcc/config/arm/arm.md
===================================================================
--- gcc/config/arm/arm.md	(revision 191088)
+++ gcc/config/arm/arm.md	(working copy)
@@ -7102,43 +7102,6 @@ 
 		(const_int 8))))]
 )
 
-;; Two peepholes to generate subtract of 0 instead of a move if the
-;; condition codes will be useful.
-(define_peephole2
-  [(set (match_operand:SI 0 "low_register_operand" "")
-	(match_operand:SI 1 "low_register_operand" ""))
-   (set (pc)
-	(if_then_else (match_operator 2 "arm_comparison_operator"
-		       [(match_dup 1) (const_int 0)])
-		      (label_ref (match_operand 3 "" ""))
-		      (pc)))]
-  "TARGET_THUMB1"
-  [(set (match_dup 0) (minus:SI (match_dup 1) (const_int 0)))
-   (set (pc)
-	(if_then_else (match_op_dup 2 [(match_dup 0) (const_int 0)])
-		      (label_ref (match_dup 3))
-		      (pc)))]
-  "")
-
-;; Sigh!  This variant shouldn't be needed, but combine often fails to
-;; merge cases like this because the op1 is a hard register in
-;; arm_class_likely_spilled_p.
-(define_peephole2
-  [(set (match_operand:SI 0 "low_register_operand" "")
-	(match_operand:SI 1 "low_register_operand" ""))
-   (set (pc)
-	(if_then_else (match_operator 2 "arm_comparison_operator"
-		       [(match_dup 0) (const_int 0)])
-		      (label_ref (match_operand 3 "" ""))
-		      (pc)))]
-  "TARGET_THUMB1"
-  [(set (match_dup 0) (minus:SI (match_dup 1) (const_int 0)))
-   (set (pc)
-	(if_then_else (match_op_dup 2 [(match_dup 0) (const_int 0)])
-		      (label_ref (match_dup 3))
-		      (pc)))]
-  "")
-
 (define_insn "*negated_cbranchsi4"
   [(set (pc)
 	(if_then_else