diff mbox

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

Message ID CABu31nO-FyF_T3KCv1PW-ndAotCKdxpjzROm7kjTEb5p2DEt3A@mail.gmail.com
State New
Headers show

Commit Message

Steven Bosscher March 20, 2015, 7:54 a.m. UTC 
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

Comments

Thomas Preud'homme March 20, 2015, 8:36 a.m. UTC | #1 
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
Thomas Preud'homme March 20, 2015, 10:27 a.m. UTC | #2 
> 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
Steven Bosscher March 20, 2015, 12:13 p.m. UTC | #3 
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
Thomas Preud'homme March 23, 2015, 11:01 a.m. UTC | #4 
> 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
Steven Bosscher March 23, 2015, 11:57 a.m. UTC | #5 
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.  */
Thomas Preud'homme March 30, 2015, 4:58 a.m. UTC | #6 
> 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
diff mbox

Patch

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