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

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

[Thomas Preud'homme]

Hi,

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. This solves the redundant ldr problem in GCC32RM-439.

ChangeLog entries are as follows:

*** gcc/ChangeLog ***

2015-01-21 Thomas Preud'homme thomas.preudhomme@arm.com

    * cprop.c (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.


*** gcc/testsuite/ChangeLog ***

2015-01-21 Thomas Preud'homme thomas.preudhomme@arm.com

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

Following testing was done:

    Bootstrapped on x86_64 and ran the testsuite without regression
    Build an arm-none-eabi cross-compilers and ran the testsuite without regression with QEMU emulating a Cortex-M3
    Compiled CSiBE targeting x86_64 and Cortex-M3 arm-none-eabi without regression

diff --git a/gcc/cprop.c b/gcc/cprop.c
index 4538291..c246d4b 100644
--- a/gcc/cprop.c
+++ b/gcc/cprop.c
@@ -815,15 +815,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
@@ -863,8 +863,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.  */
@@ -875,10 +877,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
@@ -1044,40 +1042,41 @@ 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);
+  do
+    {
+      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);
+      /* 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++)
-    {
-      rtx reg_used = reg_use_table[i];
-      unsigned int regno = REGNO (reg_used);
-      rtx src;
-      struct cprop_expr *set;
+      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];
 
-      /* 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;
+	  /* 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;
 
-      /* 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;
+	  /* Find an assignment that sets reg_used and is available
+	     at the start of the block.  */
+	  find_avail_set (regno, insn, set);
 
-      src = set->src;
+	  if (set[0])
+	    src_reg = set[0]->src;
+	  if (set[1])
+	    src_cst = set[1]->src;
 
-      /* Constant propagation.  */
-      if (cprop_constant_p (src))
-	{
-          if (constprop_register (reg_used, src, insn))
+	  /* 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++;
@@ -1087,18 +1086,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))
+	  else if (src_reg && REG_P (src_reg)
+		   && REGNO (src_reg) >= FIRST_PSEUDO_REGISTER
+		   && REGNO (src_reg) != regno
+		   && try_replace_reg (reg_used, src_reg, insn))
 	    {
 	      changed_this_round = changed = 1;
 	      global_copy_prop_count++;
@@ -1107,22 +1104,20 @@ 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
 		 deletable.  We leave the deletion to DCE.  */
-	      /* FIXME: If it turns out that the insn isn't deletable,
-		 then we may have unnecessarily extended register lifetimes
+	      /* FIXME: If it turns out that the insn isn't deletable, then we
+		 then we may have unnecessarily extended register lifetimes and
 		 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;
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 } } */

Is this ok for stage1?

Best regards,

Thomas

Re: [PATCH, GCC, stage1] Fallback to copy-prop if constant-prop not possible

[Richard Biener]

On Mon, 16 Feb 2015, Thomas Preud'homme wrote:

> Hi,
> 
> 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. This solves the redundant ldr problem in 
> GCC32RM-439.

I think Steven is more familiar with this code.

Richard.

> ChangeLog entries are as follows:
> 
> *** gcc/ChangeLog ***
> 
> 2015-01-21 Thomas Preud'homme thomas.preudhomme@arm.com
> 
>     * cprop.c (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.
> 
> 
> *** gcc/testsuite/ChangeLog ***
> 
> 2015-01-21 Thomas Preud'homme thomas.preudhomme@arm.com
> 
>     * gcc.target/arm/pr64616.c: New file.
> 
> Following testing was done:
> 
>     Bootstrapped on x86_64 and ran the testsuite without regression
>     Build an arm-none-eabi cross-compilers and ran the testsuite without regression with QEMU emulating a Cortex-M3
>     Compiled CSiBE targeting x86_64 and Cortex-M3 arm-none-eabi without regression
> 
> diff --git a/gcc/cprop.c b/gcc/cprop.c
> index 4538291..c246d4b 100644
> --- a/gcc/cprop.c
> +++ b/gcc/cprop.c
> @@ -815,15 +815,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
> @@ -863,8 +863,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.  */
> @@ -875,10 +877,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
> @@ -1044,40 +1042,41 @@ 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);
> +  do
> +    {
> +      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);
> +      /* 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++)
> -    {
> -      rtx reg_used = reg_use_table[i];
> -      unsigned int regno = REGNO (reg_used);
> -      rtx src;
> -      struct cprop_expr *set;
> +      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];
>  
> -      /* 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;
> +	  /* 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;
>  
> -      /* 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;
> +	  /* Find an assignment that sets reg_used and is available
> +	     at the start of the block.  */
> +	  find_avail_set (regno, insn, set);
>  
> -      src = set->src;
> +	  if (set[0])
> +	    src_reg = set[0]->src;
> +	  if (set[1])
> +	    src_cst = set[1]->src;
>  
> -      /* Constant propagation.  */
> -      if (cprop_constant_p (src))
> -	{
> -          if (constprop_register (reg_used, src, insn))
> +	  /* 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++;
> @@ -1087,18 +1086,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))
> +	  else if (src_reg && REG_P (src_reg)
> +		   && REGNO (src_reg) >= FIRST_PSEUDO_REGISTER
> +		   && REGNO (src_reg) != regno
> +		   && try_replace_reg (reg_used, src_reg, insn))
>  	    {
>  	      changed_this_round = changed = 1;
>  	      global_copy_prop_count++;
> @@ -1107,22 +1104,20 @@ 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
>  		 deletable.  We leave the deletion to DCE.  */
> -	      /* FIXME: If it turns out that the insn isn't deletable,
> -		 then we may have unnecessarily extended register lifetimes
> +	      /* FIXME: If it turns out that the insn isn't deletable, then we
> +		 then we may have unnecessarily extended register lifetimes and
>  		 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;
> 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 } } */
> 
> Is this ok for stage1?
> 
> Best regards,
> 
> Thomas
> 
> 
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Jennifer Guild,
Dilip Upmanyu, Graham Norton HRB 21284 (AG Nuernberg)

Re: [PATCH, GCC, stage1] Fallback to copy-prop if constant-prop not possible

[Steven Bosscher]

On Mon, Feb 16, 2015 at 11:26 AM, Thomas Preud'homme
<thomas.preudhomme@arm.com> wrote:
> Hi,
>
> 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. This solves the redundant ldr problem in GCC32RM-439.
>

This would address https://gcc.gnu.org/bugzilla/show_bug.cgi?id=34503#c4

I'll have a look at the patch tonight.

Ciao!
Seven

Re: [PATCH, GCC, stage1] Fallback to copy-prop if constant-prop not possible

[Steven Bosscher]

On Mon, Feb 16, 2015 at 11:26 AM, Thomas Preud'homme wrote:

>  /* Subroutine of cprop_insn that tries to propagate constants into
> @@ -1044,40 +1042,41 @@ cprop_insn (rtx_insn *insn)

> -      /* Constant propagation.  */
> -      if (cprop_constant_p (src))
> -       {
> -          if (constprop_register (reg_used, src, insn))
> +         /* 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++;

The cprop_constant_p test is redundant, you only have non-NULL src_cst
if it is a cprop_constant_p (as you test for it in find_avail_set()).


> @@ -1087,18 +1086,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))
> +         else if (src_reg && REG_P (src_reg)
> +                  && REGNO (src_reg) >= FIRST_PSEUDO_REGISTER
> +                  && REGNO (src_reg) != regno
> +                  && try_replace_reg (reg_used, src_reg, insn))

Likewise for the REG_P and ">= FIRST_PSEUDO_REGISTER" tests here (with
the equivalent and IMHO preferable HARD_REGISTER_P test in
find_avail_set()).


Looks good to me otherwise.

Ciao!
Steven

RE: [PATCH, GCC, stage1] Fallback to copy-prop if constant-prop not possible

[Thomas Preud'homme]

> From: Steven Bosscher [mailto:stevenb.gcc@gmail.com]
> Sent: Tuesday, February 17, 2015 4:19 AM
> To: Thomas Preud'homme
> Cc: GCC Patches; Richard Biener
> Subject: Re: [PATCH, GCC, stage1] Fallback to copy-prop if constant-prop
> not possible
> 
> On Mon, Feb 16, 2015 at 11:26 AM, Thomas Preud'homme wrote:
> 
> >  /* Subroutine of cprop_insn that tries to propagate constants into
> > @@ -1044,40 +1042,41 @@ cprop_insn (rtx_insn *insn)
> 
> > -      /* Constant propagation.  */
> > -      if (cprop_constant_p (src))
> > -       {
> > -          if (constprop_register (reg_used, src, insn))
> > +         /* 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++;
> 
> The cprop_constant_p test is redundant, you only have non-NULL
> src_cst
> if it is a cprop_constant_p (as you test for it in find_avail_set()).

Ack.

> 
> 
> > @@ -1087,18 +1086,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))
> > +         else if (src_reg && REG_P (src_reg)
> > +                  && REGNO (src_reg) >= FIRST_PSEUDO_REGISTER
> > +                  && REGNO (src_reg) != regno
> > +                  && try_replace_reg (reg_used, src_reg, insn))
> 
> Likewise for the REG_P and ">= FIRST_PSEUDO_REGISTER" tests here
> (with
> the equivalent and IMHO preferable HARD_REGISTER_P test in
> find_avail_set()).

I'm not sure I follow you here. First, it seems to me that the equivalent
test is rather REG_P && !HARD_REGISTER_P since here it checks if it's
a pseudo register.

Then, do you mean the test can be simply removed because of the
REG_P && !HARD_REGISTER_P in hash_scan_set () called indirectly by
compute_hash_table () when called in one_cprop_pass () before any
cprop_insn ()? Or do you mean I should move the check in
find_avail_set ()?

Best regards,

Thomas

RE: [PATCH, GCC, stage1] Fallback to copy-prop if constant-prop not possible

[Thomas Preud'homme]

Ping?

> From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-
> owner@gcc.gnu.org] On Behalf Of Thomas Preud'homme

[SNIP]

> >
> > Likewise for the REG_P and ">= FIRST_PSEUDO_REGISTER" tests here
> > (with
> > the equivalent and IMHO preferable HARD_REGISTER_P test in
> > find_avail_set()).
> 
> I'm not sure I follow you here. First, it seems to me that the equivalent
> test is rather REG_P && !HARD_REGISTER_P since here it checks if it's
> a pseudo register.
> 
> Then, do you mean the test can be simply removed because of the
> REG_P && !HARD_REGISTER_P in hash_scan_set () called indirectly by
> compute_hash_table () when called in one_cprop_pass () before any
> cprop_insn ()? Or do you mean I should move the check in
> find_avail_set ()?
> 
> Best regards,
> 
> Thomas
> 
> 
> 
> 

Re: [PATCH, GCC, stage1] Fallback to copy-prop if constant-prop not possible

[Steven Bosscher]

[-- Attachment #1: Type: text/plain, Size: 1834 bytes --]

On Tue, Feb 17, 2015 at 3:51 AM, Thomas Preud'homme wrote:
>> > -      else if (REG_P (src)
>> > -              && REGNO (src) >= FIRST_PSEUDO_REGISTER
>> > -              && REGNO (src) != regno)
>> > -       {
>> > -         if (try_replace_reg (reg_used, src, insn))
>> > +         else if (src_reg && REG_P (src_reg)
>> > +                  && REGNO (src_reg) >= FIRST_PSEUDO_REGISTER
>> > +                  && REGNO (src_reg) != regno
>> > +                  && try_replace_reg (reg_used, src_reg, insn))
>>
>> Likewise for the REG_P and ">= FIRST_PSEUDO_REGISTER" tests here
>> (with
>> the equivalent and IMHO preferable HARD_REGISTER_P test in
>> find_avail_set()).
>
> I'm not sure I follow you here. First, it seems to me that the equivalent
> test is rather REG_P && !HARD_REGISTER_P since here it checks if it's
> a pseudo register.
>
> Then, do you mean the test can be simply removed because of the
> REG_P && !HARD_REGISTER_P in hash_scan_set () called indirectly by
> compute_hash_table () when called in one_cprop_pass () before any
> cprop_insn ()? Or do you mean I should move the check in
> find_avail_set ()?


What I meant, is that I believe the tests are already done in
hash_scan_set and should be redundant in cprop_insn (i.e. the test can
be replaced with gcc_[checking_]assert).

I've attached a patch with some changes to it: introduce cprop_reg_p()
to get rid of all the "REG_P && regno > FIRST_PSEUDO_REGISTER" tests.
I still have the cprop_constant_p and cprop_reg_p tests in cprop_insn
but this weekend I'll try with gcc_checking_asserts instead. Please
have a look at the patch and let me know if you like it (given it's
mostly yours I hope you do like it ;-)

Bootstrapped & tested on powerpc64-unknown-linux-gnu. In building all
of cc1, 35 extra copies are propagated with the patch.

Ciao!
Steven

[-- Attachment #2: cprop_const_and_reg.diff --]
[-- Type: text/plain, Size: 7282 bytes --]

Index: cprop.c
===================================================================
--- cprop.c	(revision 221523)
+++ cprop.c	(working copy)
@@ -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 has
   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 has
 	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 @@ try_replace_reg (rtx from, rtx to, 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 @@ try_replace_reg (rtx from, rtx to, 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,7 +1055,8 @@ cprop_insn (rtx_insn *insn)
   int changed = 0, changed_this_round;
   rtx note;
 
-retry:
+  do
+    {
   changed_this_round = 0;
   reg_use_count = 0;
   note_uses (&PATTERN (insn), find_used_regs, NULL);
@@ -1064,8 +1070,8 @@ cprop_insn (rtx_insn *insn)
     {
       rtx reg_used = reg_use_table[i];
       unsigned int regno = REGNO (reg_used);
-      rtx src;
-      struct cprop_expr *set;
+	  rtx src_cst = NULL, src_reg = NULL;
+	  struct cprop_expr *set[2];
 
       /* If the register has already been set in this block, there's
 	 nothing we can do.  */
@@ -1074,17 +1080,16 @@ cprop_insn (rtx_insn *insn)
 
       /* 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;
+	  find_avail_set (regno, insn, set);
+	  if (set[0])
+	    src_reg = set[0]->src;
+	  if (set[1])
+	    src_cst = set[1]->src;
 
-      src = set->src;
-
       /* Constant propagation.  */
-      if (cprop_constant_p (src))
+	  if (src_cst && cprop_constant_p (src_cst)
+	      && constprop_register (reg_used, src_cst, insn))
 	{
-          if (constprop_register (reg_used, src, insn))
-	    {
 	      changed_this_round = changed = 1;
 	      global_const_prop_count++;
 	      if (dump_file != NULL)
@@ -1093,19 +1098,17 @@ cprop_insn (rtx_insn *insn)
 			   "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)
+	  /* Copy propagation.  */
+	  else if (src_reg && cprop_reg_p (src_reg)
+		   && REGNO (src_reg) != regno
+		   && try_replace_reg (reg_used, src_reg, insn))
 	{
-	  if (try_replace_reg (reg_used, src, insn))
-	    {
 	      changed_this_round = changed = 1;
 	      global_copy_prop_count++;
 	      if (dump_file != NULL)
@@ -1113,7 +1116,7 @@ cprop_insn (rtx_insn *insn)
 		  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 @@ cprop_insn (rtx_insn *insn)
 		 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;
-    }
+  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.  */

RE: [PATCH, GCC, stage1] Fallback to copy-prop if constant-prop not possible

[Thomas Preud'homme]

Hi Steven,

> From: Steven Bosscher [mailto:stevenb.gcc@gmail.com]
> Sent: Friday, March 20, 2015 3:54 PM
> 
> 
> What I meant, is that I believe the tests are already done in
> hash_scan_set and should be redundant in cprop_insn (i.e. the test can
> be replaced with gcc_[checking_]assert).

Ok.

> 
> I've attached a patch with some changes to it: introduce cprop_reg_p()
> to get rid of all the "REG_P && regno > FIRST_PSEUDO_REGISTER" tests.
> I still have the cprop_constant_p and cprop_reg_p tests in cprop_insn
> but this weekend I'll try with gcc_checking_asserts instead. Please
> have a look at the patch and let me know if you like it (given it's
> mostly yours I hope you do like it ;-)

I think it would be preferable to introduce PSEUDO_REG_P in rtl.h as this
seems like a common pattern enough [1]. It would be nice to have a
HARD_REG_P that would be cover the other common patterns
REG_P && < FIRST_PSEUDO_REGISTER and REG_P && HARD_REGISTER_P
but I can't come up with a good name (HARD_REGISTER_P is confusing
because it doesn't check if it's a register in the first place).

I noticed in do_local_cprop you replace >= FIRST_PSEUDO_REGISTER by
cprop_reg_p without removing the REG_P as well. In implicit_set_cond_p
there is a replacement of !REG_P || HARD_REGISTER_P by cprop_reg_p.
It seems to me it should rather be replaced by !cprop_reg_p. Otherwise it
looks ok.

[1] grep -R "REG_P .*&&.*>= FIRST_PSEUDO_REGISTER" . | wc -l returns 23

> 
> Bootstrapped & tested on powerpc64-unknown-linux-gnu. In building all
> of cc1, 35 extra copies are propagated with the patch.

I'll try to launch a build and testsuite run with these 2 issues fixed before I
leave tonight and will report the result on Monday.

Best regards,

Thomas

RE: [PATCH, GCC, stage1] Fallback to copy-prop if constant-prop not possible

[Thomas Preud'homme]

> From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-
> owner@gcc.gnu.org] On Behalf Of Thomas Preud'homme
> 
> I noticed in do_local_cprop you replace >= FIRST_PSEUDO_REGISTER by
> cprop_reg_p without removing the REG_P as well.

Sorry, I missed the parenthesis. REG_P needs indeed to be kept. I'd be
tempted to use !HARD_REGISTER_P instead since REG_P is already
checked but I don't mind either way.

Best regards,

Thomas 

Re: [PATCH, GCC, stage1] Fallback to copy-prop if constant-prop not possible

[Steven Bosscher]

On Fri, Mar 20, 2015 at 11:27 AM, Thomas Preud'homme wrote:
> Sorry, I missed the parenthesis. REG_P needs indeed to be kept. I'd be
> tempted to use !HARD_REGISTER_P instead since REG_P is already
> checked but I don't mind either way.

I put the cprop_reg_p check there instead of !HARD_REGISTER_P because
I like to be able to quickly find all places where a similar check is
performed. The check is whether the reg is something that copy
propagation can handle, and that is what I added cprop_reg_p for.
(Note that cprop can _currently_ handle only pseudos but there is no
reason why a limited set of hard regs can't be handled also, e.g. the
flag registers like in targetm.fixed_condition_code_regs).

In this case, the result is that REG_P is checked twice.
But then again, cprop_reg_p will be inlined and the double check optimized away.

Anyway, I guess we've bikeshedded long enough over this patch as it is
:-) Let's post a final form and declare it OK for stage1.

As for PSEUDO_REG_P: If it were up to me, I'd like to have in rtl.h:

static bool
hard_register_p (rtx x)
{
  return (REG_P (x) && HARD_REGISTER_NUM_P (REGNO (x)));
}

static bool
pseudo_register_p (rtx x)
{
  return (REG_P (x) && !HARD_REGISTER_NUM_P (REGNO (x)));
}

and do away with all the FIRST_PSEUDO_REGISTER tests. But I've
proposed this in the past and there was opposition. Perhaps when we
introduce a rtx_reg class...

Ciao!
Steven

RE: [PATCH, GCC, stage1] Fallback to copy-prop if constant-prop not possible

[Thomas Preud'homme]

> From: Steven Bosscher [mailto:stevenb.gcc@gmail.com]
> Sent: Friday, March 20, 2015 8:14 PM
> 
> I put the cprop_reg_p check there instead of !HARD_REGISTER_P
> because
> I like to be able to quickly find all places where a similar check is
> performed. The check is whether the reg is something that copy
> propagation can handle, and that is what I added cprop_reg_p for.

Makes sense indeed. I didn't think about the meaning of it.

> (Note that cprop can _currently_ handle only pseudos but there is no
> reason why a limited set of hard regs can't be handled also, e.g. the
> flag registers like in targetm.fixed_condition_code_regs).
> 
> In this case, the result is that REG_P is checked twice.
> But then again, cprop_reg_p will be inlined and the double check
> optimized away.

True.

> 
> Anyway, I guess we've bikeshedded long enough over this patch as it is
> :-) Let's post a final form and declare it OK for stage1.

What about the cprop_reg_p that needs to be negated? Did I miss something
that makes it ok?

> 
> As for PSEUDO_REG_P: If it were up to me, I'd like to have in rtl.h:
> 
> static bool
> hard_register_p (rtx x)
> {
>   return (REG_P (x) && HARD_REGISTER_NUM_P (REGNO (x)));
> }
> 
> static bool
> pseudo_register_p (rtx x)
> {
>   return (REG_P (x) && !HARD_REGISTER_NUM_P (REGNO (x)));
> }
> 
> and do away with all the FIRST_PSEUDO_REGISTER tests. But I've
> proposed this in the past and there was opposition. Perhaps when we
> introduce a rtx_reg class...

Ok I'll try to dig up what was the reasons presented. Anyway, it would
be done in a separate patch so not a problem for this one.

FYI testing your patch with the one cprop_reg_p negated as said in my
previous email shows no regression on arm-none-eabi cross-compiler
targeting Cortex-M3. Testing for x86_64 is ongoing.

Best regards,

Thomas

Re: [PATCH, GCC, stage1] Fallback to copy-prop if constant-prop not possible

[Steven Bosscher]

On Mon, Mar 23, 2015 at 12:01 PM, Thomas Preud'homme wrote:
> What about the cprop_reg_p that needs to be negated? Did I miss something
> that makes it ok?

You didn't miss anything.  I sent the wrong patch. The one I tested on
ppc64 also has the condition reversed:

@@ -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.  */

RE: [PATCH, GCC, stage1] Fallback to copy-prop if constant-prop not possible

[Thomas Preud'homme]

> From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-
> owner@gcc.gnu.org] On Behalf Of Thomas Preud'homme
> 
> FYI testing your patch with the one cprop_reg_p negated as said in my
> previous email shows no regression on arm-none-eabi cross-compiler
> targeting Cortex-M3. Testing for x86_64 is ongoing.

Sorry, I forgot to report back on this. No regression as well on x86_64-linux-gnu.

Do you want me to respin the patch (adding the testcase from the patch I
sent, fixing the indentation and adding a ChangeLog)?

Best regards,

Thomas

Re: [PATCH, GCC, stage1] Fallback to copy-prop if constant-prop not possible

[Jeff Law]

On 02/16/2015 03:26 AM, Thomas Preud'homme wrote:
> Hi,
>
> 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. This solves the redundant ldr problem in GCC32RM-439.
>
> ChangeLog entries are as follows:
>
> *** gcc/ChangeLog ***
>
> 2015-01-21 Thomas Preud'homme thomas.preudhomme@arm.com
>
>      * cprop.c (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.
>
>
> *** gcc/testsuite/ChangeLog ***
>
> 2015-01-21 Thomas Preud'homme thomas.preudhomme@arm.com
>
>      * gcc.target/arm/pr64616.c: New file.
Thomas,

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?

Thanks,
jeff

RE: [PATCH, GCC, stage1] Fallback to copy-prop if constant-prop not possible

[Thomas Preud'homme]

> From: Jeff Law [mailto:law@redhat.com]
> Sent: Monday, April 13, 2015 8:48 PM
> Thomas,
> 
> 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?

Sure, I'm testing it right now. Sorry for not doing it earlier, I wasn't sure what
constitute "too much disruption" as per GCC 6.0 Status Report email.

Best regards,

Thomas

RE: [PATCH, GCC, stage1] Fallback to copy-prop if constant-prop not possible

[Thomas Preud'homme]

> 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);
+}
+
 /* 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 } } */

Best regards,

Thomas

Re: [PATCH, GCC, stage1] Fallback to copy-prop if constant-prop not possible

[Steven Bosscher]

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

Re: [PATCH, GCC, stage1] Fallback to copy-prop if constant-prop not possible

[Jeff Law]

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

RE: [PATCH, GCC, stage1] Fallback to copy-prop if constant-prop not possible

[Thomas Preud'homme]

> 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

Re: [PATCH, GCC, stage1] Fallback to copy-prop if constant-prop not possible

[Jeff Law]

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

RE: [PATCH, GCC, stage1] Fallback to copy-prop if constant-prop not possible

[Thomas Preud'homme]

> 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

Re: [PATCH, GCC, stage1] Fallback to copy-prop if constant-prop not possible

[Bin.Cheng]

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
>
>
>