diff mbox

[stage1] Fallback to copy-prop if constant-prop not possible

Message ID 000001d07821$6fb82f60$4f288e20$@arm.com
State New
Headers show

Commit Message

Thomas Preud'homme April 16, 2015, 8:43 a.m. UTC 
> From: Jeff Law [mailto:law@redhat.com]
> Sent: Monday, April 13, 2015 8:48 PM
> 
> I know there were several followups between Steven and yourself.
> With
> stage1 now open, can you post a final version and do a final
> bootstrap/test with it?

Here is what came out of our discussion with Steven:

The RTL cprop pass in GCC operates by doing a local constant/copy propagation
first and then a global one. In the local one, if a constant cannot be propagated
(eg. due to constraints of the destination instruction) a copy propagation is
done instead. However, at the global level copy propagation is only tried if no
constant can be propagated, ie. if a constant can be propagated but the
constraints of the destination instruction forbids it, no copy propagation will be
tried. This patch fixes this issue.

ChangeLog entries are as follows:

*** gcc/ChangeLog ***

2015-04-15  Thomas Preud'homme  <thomas.preudhomme@arm.com>
                        Steven Bosscher <stevenb.gcc@gmail.com>

        * cprop.c (cprop_reg_p): New.
        (hash_scan_set): Use above function to check if register can be
        propagated.
        (find_avail_set): Return up to two sets, one whose source is
        a register and one whose source is a constant.  Sets are returned in
        an array passed as parameter rather than as a return value.
        (cprop_insn): Use a do while loop rather than a goto.  Try each of the
        sets returned by find_avail_set, starting with the one whose source is
        a constant. Use cprop_reg_p to check if register can be propagated.
        (do_local_cprop): Use cprop_reg_p to check if register can be
        propagated.
        (implicit_set_cond_p): Likewise.

*** gcc/testsuite/ChangeLog ***

2015-04-15  Thomas Preud'homme  <thomas.preudhomme@arm.com>
                        Steven Bosscher <stevenb.gcc@gmail.com>

        * gcc.target/arm/pr64616.c: New file.


And the patch is:



Best regards,

Thomas

Comments

Steven Bosscher April 23, 2015, 9:14 a.m. UTC | #1 
On Thu, Apr 16, 2015 at 10:43 AM, Thomas Preud'homme wrote:
> 2015-04-15  Thomas Preud'homme  <thomas.preudhomme@arm.com>
>                         Steven Bosscher <stevenb.gcc@gmail.com>
>
>         * cprop.c (cprop_reg_p): New.
>         (hash_scan_set): Use above function to check if register can be
>         propagated.
>         (find_avail_set): Return up to two sets, one whose source is
>         a register and one whose source is a constant.  Sets are returned in
>         an array passed as parameter rather than as a return value.
>         (cprop_insn): Use a do while loop rather than a goto.  Try each of the
>         sets returned by find_avail_set, starting with the one whose source is
>         a constant. Use cprop_reg_p to check if register can be propagated.
>         (do_local_cprop): Use cprop_reg_p to check if register can be
>         propagated.
>         (implicit_set_cond_p): Likewise.


I wouldn't usually approve patches I've coded bits in myself. But this
post is now 7 days old and it's Thomas' patch for 99%, so...

OK for trunk.

Can you please put steven at gcc.gnu.org for my e-mail address in the
ChangeLog entry?

Ciao!
Steven
Jeff Law April 24, 2015, 2:59 a.m. UTC | #2 
On 04/16/2015 02:43 AM, Thomas Preud'homme wrote:
>> From: Jeff Law [mailto:law@redhat.com]
>> Sent: Monday, April 13, 2015 8:48 PM
>>
>> I know there were several followups between Steven and yourself.
>> With
>> stage1 now open, can you post a final version and do a final
>> bootstrap/test with it?
>
> Here is what came out of our discussion with Steven:
>
> The RTL cprop pass in GCC operates by doing a local constant/copy propagation
> first and then a global one. In the local one, if a constant cannot be propagated
> (eg. due to constraints of the destination instruction) a copy propagation is
> done instead. However, at the global level copy propagation is only tried if no
> constant can be propagated, ie. if a constant can be propagated but the
> constraints of the destination instruction forbids it, no copy propagation will be
> tried. This patch fixes this issue.
>
> ChangeLog entries are as follows:
>
> *** gcc/ChangeLog ***
>
> 2015-04-15  Thomas Preud'homme  <thomas.preudhomme@arm.com>
>                          Steven Bosscher <stevenb.gcc@gmail.com>
>
>          * cprop.c (cprop_reg_p): New.
>          (hash_scan_set): Use above function to check if register can be
>          propagated.
>          (find_avail_set): Return up to two sets, one whose source is
>          a register and one whose source is a constant.  Sets are returned in
>          an array passed as parameter rather than as a return value.
>          (cprop_insn): Use a do while loop rather than a goto.  Try each of the
>          sets returned by find_avail_set, starting with the one whose source is
>          a constant. Use cprop_reg_p to check if register can be propagated.
>          (do_local_cprop): Use cprop_reg_p to check if register can be
>          propagated.
>          (implicit_set_cond_p): Likewise.
>
> *** gcc/testsuite/ChangeLog ***
>
> 2015-04-15  Thomas Preud'homme  <thomas.preudhomme@arm.com>
>                          Steven Bosscher <stevenb.gcc@gmail.com>
>
>          * gcc.target/arm/pr64616.c: New file.
>
>
> And the patch is:
>
>
> diff --git a/gcc/cprop.c b/gcc/cprop.c
> index c9fb2fc..78541cf 100644
> --- a/gcc/cprop.c
> +++ b/gcc/cprop.c
> @@ -285,6 +285,15 @@ cprop_constant_p (const_rtx x)
>     return CONSTANT_P (x) && (GET_CODE (x) != CONST || shared_const_p (x));
>   }
>
> +/* Determine whether the rtx X should be treated as a register that can
> +   be propagated.  Any pseudo-register is fine.  */
> +
> +static bool
> +cprop_reg_p (const_rtx x)
> +{
> +  return REG_P (x) && !HARD_REGISTER_P (x);
> +}
How about instead this move to a more visible location (perhaps a macro 
in regs.h or an inline function).  Then as a followup, change the 
various places that have this sequence to use that common definition 
that exist outside of cprop.c.

> @@ -1191,7 +1192,7 @@ do_local_cprop (rtx x, rtx_insn *insn)
>     /* Rule out USE instructions and ASM statements as we don't want to
>        change the hard registers mentioned.  */
>     if (REG_P (x)
> -      && (REGNO (x) >= FIRST_PSEUDO_REGISTER
> +      && (cprop_reg_p (x)
>             || (GET_CODE (PATTERN (insn)) != USE
>   	      && asm_noperands (PATTERN (insn)) < 0)))
Isn't the REG_P test now redundant?

OK for the trunk with those changes.

jeff
Thomas Preud'homme April 24, 2015, 3:10 a.m. UTC | #3 
> From: Jeff Law [mailto:law@redhat.com]
> Sent: Friday, April 24, 2015 10:59 AM
> 

Hi Jeff,

> > +
> > +static bool
> > +cprop_reg_p (const_rtx x)
> > +{
> > +  return REG_P (x) && !HARD_REGISTER_P (x);
> > +}
> How about instead this move to a more visible location (perhaps a macro
> in regs.h or an inline function).  Then as a followup, change the
> various places that have this sequence to use that common definition
> that exist outside of cprop.c.

According to Steven this was proposed in the past but was refused (see
end of [1]).

[1] https://gcc.gnu.org/ml/gcc-patches/2015-03/msg01066.html

> 
> > @@ -1191,7 +1192,7 @@ do_local_cprop (rtx x, rtx_insn *insn)
> >     /* Rule out USE instructions and ASM statements as we don't want
> to
> >        change the hard registers mentioned.  */
> >     if (REG_P (x)
> > -      && (REGNO (x) >= FIRST_PSEUDO_REGISTER
> > +      && (cprop_reg_p (x)
> >             || (GET_CODE (PATTERN (insn)) != USE
> >   	      && asm_noperands (PATTERN (insn)) < 0)))
> Isn't the REG_P test now redundant?

I made the same mistake when reviewing that change and indeed it's not.
Note the opening parenthesis before cprop_reg_p that contains a bitwise
OR expression. So in the case where cprop_reg_p is false, REG_P still
needs to be true.

We could keep a check on FIRST_PSEUDO_REGISTER but the intent (checking
that the register is suitable for propagation) is clearer now, as pointed out by
Steven to me.

> 
> OK for the trunk with those changes.
> 
> jeff

Given the above I intent to keep the REG_P in the second excerpt and will
wait for your input about moving cprop_reg_p to rtl.h

Best regards,

Thomas
Jeff Law April 24, 2015, 3:14 a.m. UTC | #4 
On 04/23/2015 09:10 PM, Thomas Preud'homme wrote:
>> From: Jeff Law [mailto:law@redhat.com]
>> Sent: Friday, April 24, 2015 10:59 AM
>>
>
> Hi Jeff,
>
>>> +
>>> +static bool
>>> +cprop_reg_p (const_rtx x)
>>> +{
>>> +  return REG_P (x) && !HARD_REGISTER_P (x);
>>> +}
>> How about instead this move to a more visible location (perhaps a macro
>> in regs.h or an inline function).  Then as a followup, change the
>> various places that have this sequence to use that common definition
>> that exist outside of cprop.c.
>
> According to Steven this was proposed in the past but was refused (see
> end of [1]).
>
> [1] https://gcc.gnu.org/ml/gcc-patches/2015-03/msg01066.html
Missed that message.  Given we've already gone round and round on it, 
let go with the patch as-is and deal with hard_register_p and 
pseudo_register_p independently.  No idea who objected to that, seems 
like a no-brainer to me.

>
>>
>>> @@ -1191,7 +1192,7 @@ do_local_cprop (rtx x, rtx_insn *insn)
>>>      /* Rule out USE instructions and ASM statements as we don't want
>> to
>>>         change the hard registers mentioned.  */
>>>      if (REG_P (x)
>>> -      && (REGNO (x) >= FIRST_PSEUDO_REGISTER
>>> +      && (cprop_reg_p (x)
>>>              || (GET_CODE (PATTERN (insn)) != USE
>>>    	      && asm_noperands (PATTERN (insn)) < 0)))
>> Isn't the REG_P test now redundant?
>
> I made the same mistake when reviewing that change and indeed it's not.
> Note the opening parenthesis before cprop_reg_p that contains a bitwise
> OR expression. So in the case where cprop_reg_p is false, REG_P still
> needs to be true.
>
> We could keep a check on FIRST_PSEUDO_REGISTER but the intent (checking
> that the register is suitable for propagation) is clearer now, as pointed out by
> Steven to me.
Ah.  Nevermind then.

So revised review is "ok for the trunk" :-)

jeff
Thomas Preud'homme April 24, 2015, 4:52 a.m. UTC | #5 
> From: Jeff Law [mailto:law@redhat.com]
> Sent: Friday, April 24, 2015 11:15 AM
> 
> So revised review is "ok for the trunk" :-)

Committed.

Best regards,

Thomas
Bin.Cheng April 30, 2015, 7:28 a.m. UTC | #6 
On Fri, Apr 24, 2015 at 12:52 PM, Thomas Preud'homme
<thomas.preudhomme@arm.com> wrote:
>> From: Jeff Law [mailto:law@redhat.com]
>> Sent: Friday, April 24, 2015 11:15 AM
>>
>> So revised review is "ok for the trunk" :-)
>
> Committed.
Hi Thomas,
The newly introduced test failed on
arm-none-linux-gnueabi&arm-none-linux-gnueabihf.  Could you please
have a look at it?
FAIL: gcc.target/arm/pr64616.c scan-assembler-times ldr 2

GCC was configured with
gcc/configure --target=arm-none-linux-gnueabi --prefix=
--with-sysroot=... --enable-shared --disable-libsanitizer
--disable-libssp --disable-libmudflap
--with-plugin-ld=arm-none-linux-gnueabi-ld --enable-checking=yes
--enable-languages=c,c++,fortran --with-gmp=... --with-mpfr=...
--with-mpc=... --with-isl=... --with-cloog=... --with-arch=armv7-a
--with-fpu=vfpv3-d16 --with-float=softfp --with-arch=armv7-a

Thanks,
bin

>
> Best regards,
>
> Thomas
>
>
>
diff mbox

Patch

diff --git a/gcc/cprop.c b/gcc/cprop.c
index c9fb2fc..78541cf 100644
--- a/gcc/cprop.c
+++ b/gcc/cprop.c
@@ -285,6 +285,15 @@  cprop_constant_p (const_rtx x)
   return CONSTANT_P (x) && (GET_CODE (x) != CONST || shared_const_p (x));
 }
 
+/* Determine whether the rtx X should be treated as a register that can
+   be propagated.  Any pseudo-register is fine.  */
+
+static bool
+cprop_reg_p (const_rtx x)
+{
+  return REG_P (x) && !HARD_REGISTER_P (x);
+}
+
 /* Scan SET present in INSN and add an entry to the hash TABLE.
    IMPLICIT is true if it's an implicit set, false otherwise.  */
 
@@ -295,8 +304,7 @@  hash_scan_set (rtx set, rtx_insn *insn, struct hash_table_d *table,
   rtx src = SET_SRC (set);
   rtx dest = SET_DEST (set);
 
-  if (REG_P (dest)
-      && ! HARD_REGISTER_P (dest)
+  if (cprop_reg_p (dest)
       && reg_available_p (dest, insn)
       && can_copy_p (GET_MODE (dest)))
     {
@@ -321,9 +329,8 @@  hash_scan_set (rtx set, rtx_insn *insn, struct hash_table_d *table,
 	src = XEXP (note, 0), set = gen_rtx_SET (VOIDmode, dest, src);
 
       /* Record sets for constant/copy propagation.  */
-      if ((REG_P (src)
+      if ((cprop_reg_p (src)
 	   && src != dest
-	   && ! HARD_REGISTER_P (src)
 	   && reg_available_p (src, insn))
 	  || cprop_constant_p (src))
 	insert_set_in_table (dest, src, insn, table, implicit);
@@ -821,15 +828,15 @@  try_replace_reg (rtx from, rtx to, rtx_insn *insn)
   return success;
 }
 
-/* Find a set of REGNOs that are available on entry to INSN's block.  Return
-   NULL no such set is found.  */
+/* Find a set of REGNOs that are available on entry to INSN's block.  If found,
+   SET_RET[0] will be assigned a set with a register source and SET_RET[1] a
+   set with a constant source.  If not found the corresponding entry is set to
+   NULL.  */
 
-static struct cprop_expr *
-find_avail_set (int regno, rtx_insn *insn)
+static void
+find_avail_set (int regno, rtx_insn *insn, struct cprop_expr *set_ret[2])
 {
-  /* SET1 contains the last set found that can be returned to the caller for
-     use in a substitution.  */
-  struct cprop_expr *set1 = 0;
+  set_ret[0] = set_ret[1] = NULL;
 
   /* Loops are not possible here.  To get a loop we would need two sets
      available at the start of the block containing INSN.  i.e. we would
@@ -869,8 +876,10 @@  find_avail_set (int regno, rtx_insn *insn)
          If the source operand changed, we may still use it for the next
          iteration of this loop, but we may not use it for substitutions.  */
 
-      if (cprop_constant_p (src) || reg_not_set_p (src, insn))
-	set1 = set;
+      if (cprop_constant_p (src))
+	set_ret[1] = set;
+      else if (reg_not_set_p (src, insn))
+	set_ret[0] = set;
 
       /* If the source of the set is anything except a register, then
 	 we have reached the end of the copy chain.  */
@@ -881,10 +890,6 @@  find_avail_set (int regno, rtx_insn *insn)
 	 and see if we have an available copy into SRC.  */
       regno = REGNO (src);
     }
-
-  /* SET1 holds the last set that was available and anticipatable at
-     INSN.  */
-  return set1;
 }
 
 /* Subroutine of cprop_insn that tries to propagate constants into
@@ -1050,40 +1055,40 @@  cprop_insn (rtx_insn *insn)
   int changed = 0, changed_this_round;
   rtx note;
 
-retry:
-  changed_this_round = 0;
-  reg_use_count = 0;
-  note_uses (&PATTERN (insn), find_used_regs, NULL);
-
-  /* We may win even when propagating constants into notes.  */
-  note = find_reg_equal_equiv_note (insn);
-  if (note)
-    find_used_regs (&XEXP (note, 0), NULL);
-
-  for (i = 0; i < reg_use_count; i++)
+  do
     {
-      rtx reg_used = reg_use_table[i];
-      unsigned int regno = REGNO (reg_used);
-      rtx src;
-      struct cprop_expr *set;
+      changed_this_round = 0;
+      reg_use_count = 0;
+      note_uses (&PATTERN (insn), find_used_regs, NULL);
 
-      /* If the register has already been set in this block, there's
-	 nothing we can do.  */
-      if (! reg_not_set_p (reg_used, insn))
-	continue;
+      /* We may win even when propagating constants into notes.  */
+      note = find_reg_equal_equiv_note (insn);
+      if (note)
+	find_used_regs (&XEXP (note, 0), NULL);
 
-      /* Find an assignment that sets reg_used and is available
-	 at the start of the block.  */
-      set = find_avail_set (regno, insn);
-      if (! set)
-	continue;
+      for (i = 0; i < reg_use_count; i++)
+	{
+	  rtx reg_used = reg_use_table[i];
+	  unsigned int regno = REGNO (reg_used);
+	  rtx src_cst = NULL, src_reg = NULL;
+	  struct cprop_expr *set[2];
 
-      src = set->src;
+	  /* If the register has already been set in this block, there's
+	     nothing we can do.  */
+	  if (! reg_not_set_p (reg_used, insn))
+	    continue;
 
-      /* Constant propagation.  */
-      if (cprop_constant_p (src))
-	{
-          if (constprop_register (reg_used, src, insn))
+	  /* Find an assignment that sets reg_used and is available
+	     at the start of the block.  */
+	  find_avail_set (regno, insn, set);
+	  if (set[0])
+	    src_reg = set[0]->src;
+	  if (set[1])
+	    src_cst = set[1]->src;
+
+	  /* Constant propagation.  */
+	  if (src_cst && cprop_constant_p (src_cst)
+	      && constprop_register (reg_used, src_cst, insn))
 	    {
 	      changed_this_round = changed = 1;
 	      global_const_prop_count++;
@@ -1093,18 +1098,16 @@  retry:
 			   "GLOBAL CONST-PROP: Replacing reg %d in ", regno);
 		  fprintf (dump_file, "insn %d with constant ",
 			   INSN_UID (insn));
-		  print_rtl (dump_file, src);
+		  print_rtl (dump_file, src_cst);
 		  fprintf (dump_file, "\n");
 		}
 	      if (insn->deleted ())
 		return 1;
 	    }
-	}
-      else if (REG_P (src)
-	       && REGNO (src) >= FIRST_PSEUDO_REGISTER
-	       && REGNO (src) != regno)
-	{
-	  if (try_replace_reg (reg_used, src, insn))
+	  /* Copy propagation.  */
+	  else if (src_reg && cprop_reg_p (src_reg)
+		   && REGNO (src_reg) != regno
+		   && try_replace_reg (reg_used, src_reg, insn))
 	    {
 	      changed_this_round = changed = 1;
 	      global_copy_prop_count++;
@@ -1113,7 +1116,7 @@  retry:
 		  fprintf (dump_file,
 			   "GLOBAL COPY-PROP: Replacing reg %d in insn %d",
 			   regno, INSN_UID (insn));
-		  fprintf (dump_file, " with reg %d\n", REGNO (src));
+		  fprintf (dump_file, " with reg %d\n", REGNO (src_reg));
 		}
 
 	      /* The original insn setting reg_used may or may not now be
@@ -1123,12 +1126,10 @@  retry:
 		 and made things worse.  */
 	    }
 	}
-
-      /* If try_replace_reg simplified the insn, the regs found
-	 by find_used_regs may not be valid anymore.  Start over.  */
-      if (changed_this_round)
-	goto retry;
     }
+  /* If try_replace_reg simplified the insn, the regs found by find_used_regs
+     may not be valid anymore.  Start over.  */
+  while (changed_this_round);
 
   if (changed && DEBUG_INSN_P (insn))
     return 0;
@@ -1191,7 +1192,7 @@  do_local_cprop (rtx x, rtx_insn *insn)
   /* Rule out USE instructions and ASM statements as we don't want to
      change the hard registers mentioned.  */
   if (REG_P (x)
-      && (REGNO (x) >= FIRST_PSEUDO_REGISTER
+      && (cprop_reg_p (x)
           || (GET_CODE (PATTERN (insn)) != USE
 	      && asm_noperands (PATTERN (insn)) < 0)))
     {
@@ -1207,7 +1208,7 @@  do_local_cprop (rtx x, rtx_insn *insn)
 
 	  if (cprop_constant_p (this_rtx))
 	    newcnst = this_rtx;
-	  if (REG_P (this_rtx) && REGNO (this_rtx) >= FIRST_PSEUDO_REGISTER
+	  if (cprop_reg_p (this_rtx)
 	      /* Don't copy propagate if it has attached REG_EQUIV note.
 		 At this point this only function parameters should have
 		 REG_EQUIV notes and if the argument slot is used somewhere
@@ -1328,9 +1329,8 @@  implicit_set_cond_p (const_rtx cond)
   if (GET_CODE (cond) != EQ && GET_CODE (cond) != NE)
     return false;
 
-  /* The first operand of COND must be a pseudo-reg.  */
-  if (! REG_P (XEXP (cond, 0))
-      || HARD_REGISTER_P (XEXP (cond, 0)))
+  /* The first operand of COND must be a register we can propagate.  */
+  if (!cprop_reg_p (XEXP (cond, 0)))
     return false;
 
   /* The second operand of COND must be a suitable constant.  */
diff --git a/gcc/testsuite/gcc.target/arm/pr64616.c b/gcc/testsuite/gcc.target/arm/pr64616.c
new file mode 100644
index 0000000..c686ffa
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/pr64616.c
@@ -0,0 +1,14 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+int f (int);
+unsigned int glob;
+
+void
+g ()
+{
+  while (f (glob));
+  glob = 5;
+}
+
+/* { dg-final { scan-assembler-times "ldr" 2 } } */