Fix postreload-gcse treatment of call-clobbered registers

Fix postreload-gcse treatment of call-clobbered registers

[Richard Sandiford]

This is the patch that inspired the HARD_REG_SET constructs I just posted
(and it depends on them).  postreload-gcse.c was only checking whether
REGNO (x) is call-clobbered, but it should check whether the whole
register is.

I verified that this bug is still present on dataflow-branch.
I tripped over it while doing some other changes, so I don't have
a mainline testcase.  Hopefully the fix is obvious enough not to
need one.

Bootstrapped & regression-tested on x86_64-linux-gnu.  Also
regression-tested on sh-elf (,-m2,-m3,-m4,-m4/-ml) and built
on arm-elf.  OK to install?

Richard


gcc/
	* postreload-gcse.c (reg_set_between_after_reload_p): Check
	whether any part of a multi-register value is call-clobbered.
	(reg_used_between_after_reload_p): Likewise.

Index: gcc/postreload-gcse.c
===================================================================
--- gcc/postreload-gcse.c	2007-05-21 00:23:48.000000000 +0100
+++ gcc/postreload-gcse.c	2007-05-21 00:26:28.000000000 +0100
@@ -880,7 +880,8 @@ reg_set_between_after_reload_p (rtx reg,
 	if (set_of (reg, insn) != NULL_RTX)
 	  return insn;
 	if ((CALL_P (insn)
-	      && call_used_regs[REGNO (reg)])
+	     && overlaps_hard_reg_set_p (call_used_reg_set,
+					 GET_MODE (reg), REGNO (reg)))
 	    || find_reg_fusage (insn, CLOBBER, reg))
 	  return insn;
 
@@ -913,7 +914,8 @@ reg_used_between_after_reload_p (rtx reg
       {
 	if (reg_overlap_mentioned_p (reg, PATTERN (insn))
 	    || (CALL_P (insn)
-		&& call_used_regs[REGNO (reg)])
+		&& overlaps_hard_reg_set_p (call_used_reg_set,
+					    GET_MODE (reg), REGNO (reg)))
 	    || find_reg_fusage (insn, USE, reg)
 	    || find_reg_fusage (insn, CLOBBER, reg))
 	  return insn;

Re: Fix postreload-gcse treatment of call-clobbered registers

[Eric Botcazou]

> I tripped over it while doing some other changes, so I don't have
> a mainline testcase.  Hopefully the fix is obvious enough not to
> need one.

Could you say which part of the change fixes the bug?  Because look at what 
reg_set_between and reg_used_between do in rtlanal.c:

> Index: gcc/postreload-gcse.c
> ===================================================================
> --- gcc/postreload-gcse.c	2007-05-21 00:23:48.000000000 +0100
> +++ gcc/postreload-gcse.c	2007-05-21 00:26:28.000000000 +0100
> @@ -880,7 +880,8 @@ reg_set_between_after_reload_p (rtx reg,
>  	if (set_of (reg, insn) != NULL_RTX)
>  	  return insn;
>  	if ((CALL_P (insn)
> -	      && call_used_regs[REGNO (reg)])
> +	     && overlaps_hard_reg_set_p (call_used_reg_set,
> +					 GET_MODE (reg), REGNO (reg)))
>
>  	    || find_reg_fusage (insn, CLOBBER, reg))
>
>  	  return insn;

/* Nonzero if register REG is set or clobbered in an insn between
   FROM_INSN and TO_INSN (exclusive of those two).  */

int
reg_set_between_p (rtx reg, rtx from_insn, rtx to_insn)
{
  rtx insn;

  if (from_insn == to_insn)
    return 0;

  for (insn = NEXT_INSN (from_insn); insn != to_insn; insn = NEXT_INSN (insn))
    if (INSN_P (insn) && reg_set_p (reg, insn))
      return 1;
  return 0;
}

/* Internals of reg_set_between_p.  */
int
reg_set_p (rtx reg, rtx insn)
{
  /* We can be passed an insn or part of one.  If we are passed an insn,
     check if a side-effect of the insn clobbers REG.  */
  if (INSN_P (insn)
      && (FIND_REG_INC_NOTE (insn, reg)
	  || (CALL_P (insn)
	      && ((REG_P (reg)
		   && REGNO (reg) < FIRST_PSEUDO_REGISTER
		   && TEST_HARD_REG_BIT (regs_invalidated_by_call,
					 REGNO (reg)))
		  || MEM_P (reg)
		  || find_reg_fusage (insn, CLOBBER, reg)))))
    return 1;

  return set_of (reg, insn) != NULL_RTX;
}

> @@ -913,7 +914,8 @@ reg_used_between_after_reload_p (rtx reg
>        {
>  	if (reg_overlap_mentioned_p (reg, PATTERN (insn))
>
>  	    || (CALL_P (insn)
>
> -		&& call_used_regs[REGNO (reg)])
> +		&& overlaps_hard_reg_set_p (call_used_reg_set,
> +					    GET_MODE (reg), REGNO (reg)))
>
>  	    || find_reg_fusage (insn, USE, reg)
>  	    || find_reg_fusage (insn, CLOBBER, reg))
>
>  	  return insn;

/* Nonzero if register REG is used in an insn between
   FROM_INSN and TO_INSN (exclusive of those two).  */

int
reg_used_between_p (rtx reg, rtx from_insn, rtx to_insn)
{
  rtx insn;

  if (from_insn == to_insn)
    return 0;

  for (insn = NEXT_INSN (from_insn); insn != to_insn; insn = NEXT_INSN (insn))
    if (INSN_P (insn)
	&& (reg_overlap_mentioned_p (reg, PATTERN (insn))
	   || (CALL_P (insn) && find_reg_fusage (insn, USE, reg))))
      return 1;
  return 0;
}

-- 
Eric Botcazou

Re: Fix postreload-gcse treatment of call-clobbered registers

[Richard Sandiford]

Eric Botcazou <ebotcazou@libertysurf.fr> writes:
>> I tripped over it while doing some other changes, so I don't have
>> a mainline testcase.  Hopefully the fix is obvious enough not to
>> need one.
>
> Could you say which part of the change fixes the bug?

You mean, is one of the reg_set_* or reg_used_* changes enough on its
own to fix the bug?  I wasn't sure before I started writing this message
(although I have an inkling now).  TBH, once I found that postreload-gcse
was moving multi-register, partly call-clobbered quantities across the call,
I modified both sites at once, and the two changes together did fix the bug.

I could probably reproduce the environment that caused the bug, but it
seemd fairly obvious that both functions were wrong.  In this context,
there's nothing special about the first register compared with the rest.

However, your message seemed be implying that at least one function
was wrong in a more fundamental way, and that might indeed be the case...

> Because look at what reg_set_between and reg_used_between do in rtlanal.c:
>
>> Index: gcc/postreload-gcse.c
>> ===================================================================
>> --- gcc/postreload-gcse.c	2007-05-21 00:23:48.000000000 +0100
>> +++ gcc/postreload-gcse.c	2007-05-21 00:26:28.000000000 +0100
>> @@ -880,7 +880,8 @@ reg_set_between_after_reload_p (rtx reg,
>>  	if (set_of (reg, insn) != NULL_RTX)
>>  	  return insn;
>>  	if ((CALL_P (insn)
>> -	      && call_used_regs[REGNO (reg)])
>> +	     && overlaps_hard_reg_set_p (call_used_reg_set,
>> +					 GET_MODE (reg), REGNO (reg)))
>>
>>  	    || find_reg_fusage (insn, CLOBBER, reg))
>>
>>  	  return insn;
>
> /* Nonzero if register REG is set or clobbered in an insn between
>    FROM_INSN and TO_INSN (exclusive of those two).  */
>
> int
> reg_set_between_p (rtx reg, rtx from_insn, rtx to_insn)
> {
>   rtx insn;
>
>   if (from_insn == to_insn)
>     return 0;
>
>   for (insn = NEXT_INSN (from_insn); insn != to_insn; insn = NEXT_INSN (insn))
>     if (INSN_P (insn) && reg_set_p (reg, insn))
>       return 1;
>   return 0;
> }
>
> /* Internals of reg_set_between_p.  */
> int
> reg_set_p (rtx reg, rtx insn)
> {
>   /* We can be passed an insn or part of one.  If we are passed an insn,
>      check if a side-effect of the insn clobbers REG.  */
>   if (INSN_P (insn)
>       && (FIND_REG_INC_NOTE (insn, reg)
> 	  || (CALL_P (insn)
> 	      && ((REG_P (reg)
> 		   && REGNO (reg) < FIRST_PSEUDO_REGISTER
> 		   && TEST_HARD_REG_BIT (regs_invalidated_by_call,
> 					 REGNO (reg)))
> 		  || MEM_P (reg)
> 		  || find_reg_fusage (insn, CLOBBER, reg)))))
>     return 1;
>
>   return set_of (reg, insn) != NULL_RTX;
> }

Hmm.  You mean that reg_set_p needs the same treatment?  Looks like it
might.  I think what the patch does to reg_set_between_after_reload_p
really is right here.

>> @@ -913,7 +914,8 @@ reg_used_between_after_reload_p (rtx reg
>>        {
>>  	if (reg_overlap_mentioned_p (reg, PATTERN (insn))
>>
>>  	    || (CALL_P (insn)
>>
>> -		&& call_used_regs[REGNO (reg)])
>> +		&& overlaps_hard_reg_set_p (call_used_reg_set,
>> +					    GET_MODE (reg), REGNO (reg)))
>>
>>  	    || find_reg_fusage (insn, USE, reg)
>>  	    || find_reg_fusage (insn, CLOBBER, reg))
>>
>>  	  return insn;
>
> /* Nonzero if register REG is used in an insn between
>    FROM_INSN and TO_INSN (exclusive of those two).  */
>
> int
> reg_used_between_p (rtx reg, rtx from_insn, rtx to_insn)
> {
>   rtx insn;
>
>   if (from_insn == to_insn)
>     return 0;
>
>   for (insn = NEXT_INSN (from_insn); insn != to_insn; insn = NEXT_INSN (insn))
>     if (INSN_P (insn)
> 	&& (reg_overlap_mentioned_p (reg, PATTERN (insn))
> 	   || (CALL_P (insn) && find_reg_fusage (insn, USE, reg))))
>       return 1;
>   return 0;
> }

To be honest, I'm not sure what you mean.  That reg_used_between_p
should do the same thing?

Hmm.  I notice there's a subtle difference between the comments:

/* Return the insn that sets register REG or clobbers it in between
   FROM_INSN and TO_INSN (exclusive of those two).
   Just like reg_set_between but for hard registers and not pseudos.  */

static rtx
reg_set_between_after_reload_p (rtx reg, rtx from_insn, rtx to_insn)

[...]

/* Return the insn that uses register REG in between FROM_INSN and TO_INSN
   (exclusive of those two). Similar to reg_used_between but for hard
   registers and not pseudos.  */

static rtx
reg_used_between_after_reload_p (rtx reg, rtx from_insn, rtx to_insn)

("Just like" vs. "Similar to") which suggests that the *_use_*
functions aren't supposed to be entirely like-for-like.

To be honest, I find the only use of reg_used_between_after_reload_p
a little strange:

  if (reg_avail_info[REGNO (reg)] != 0)
    return true;

  insn = reg_used_between_after_reload_p (reg, start, up_to_insn);
  if (! insn)
    insn = reg_set_between_after_reload_p (reg, start, up_to_insn);

  if (insn)
    reg_avail_info[REGNO (reg)] = INSN_CUID (insn);

Why are we recording a _use_ in reg_avail_info?  I wouldn't have thought
that a use of REG should cause oprs_unchanged_p (REG, ..., false) to
return false for source values involving REG.  It seems like this code
is reusing reg_avail_info for what is logically a separate HARD_REG_SET.
Of course:

  if (reg_avail_info[REGNO (reg)] != 0)
    return true;

is still a useful check as well.  (Not that this affects the bug at hand.
Call-clobbered registers are, well, clobbered, so should be entered into
the array anyway.)

That said, why is reg_set_between_after_reload_p needed at all,
given that we have already passed all insns through record_opr_changes:

	  /* Keep track of everything modified by this insn, so that we
	     know what has been modified since the start of the current
	     basic block.  */
	  if (INSN_P (insn))
	    record_opr_changes (insn);

?

Given that this is the only caller of reg_used_between_after_reload_p,
and that the actual value of reg_avail_info doesn't matter in this
context (its value is only used when testing oprs_unchanged_p (..., true)
rather than oprs_unchanged_p (..., false)), I suppose that
reg_used_between_after_reload_p doesn't need to check for
call-clobberedness at all.  It will be covered by the
reg_set_between_after_reload_p check or (if I'm right above)
the first reg_avail_info check.

So having written that, I suspect that the answer to your original
question is that it's the reg_set_between_after_reload_p part that
really matters, and it's the other caller:

	  if (! reg_set_between_after_reload_p (avail_reg, avail_insn,
						next_pred_bb_end))
	    /* AVAIL_INSN remains non-null.  */
	    break;
	  else
	    avail_insn = NULL;

that was being led astray.

I have a nasty suspicion I've talked myself into more invasive changes
than the surface fix (which I still believe is correct as far as it goes).
Problem is, I'm not sure I understand the intent of the original authors
enough to rework the code in a more drastic way.  (It does all seem to
be about implementation details rather than the textbook algorithm.)

Richard

Re: Fix postreload-gcse treatment of call-clobbered registers

[Eric Botcazou]

> You mean, is one of the reg_set_* or reg_used_* changes enough on its
> own to fix the bug?  I wasn't sure before I started writing this message
> (although I have an inkling now).

Yes, that's what I meant.

> However, your message seemed be implying that at least one function
> was wrong in a more fundamental way, and that might indeed be the case...

Both functions look a little questionable to me.

> Hmm.  You mean that reg_set_p needs the same treatment?  Looks like it
> might.  I think what the patch does to reg_set_between_after_reload_p
> really is right here.

Yes, I think that reg_set_p would need the same treatment.

However, there is an existing discrepancy between them: while reg_set_p tests 
regs_invalidated_by_call, its counterpart reg_set_between_after_reload_p 
tests call_used_regs.  Moreover, in the same file, record_opr_changes also 
tests regs_invalidated_by_call for exactly the same purpose!

In other words, I think that reg_set_between_after_reload_p should test 
regs_invalidated_by_call like the 2 others.  Finally, record_opr_changes 
should also be modified along the lines of your change.

> To be honest, I'm not sure what you mean.  That reg_used_between_p
> should do the same thing?

reg_used_between_p already takes into account multi-word hard regs through the 
helper function find_reg_fusage.

It seems to me that reg_used_between_after_reload_p has been copy-and-pasted 
from reg_set_between_after_reload_p a little quickly:
- why testing call_used_regs?
- why testing CLOBBERs?

I think that the correct condition is that of reg_used_between_p.

> To be honest, I find the only use of reg_used_between_after_reload_p
> a little strange:
>
>   if (reg_avail_info[REGNO (reg)] != 0)
>     return true;
>
>   insn = reg_used_between_after_reload_p (reg, start, up_to_insn);
>   if (! insn)
>     insn = reg_set_between_after_reload_p (reg, start, up_to_insn);
>
>   if (insn)
>     reg_avail_info[REGNO (reg)] = INSN_CUID (insn);
>
> Why are we recording a _use_ in reg_avail_info?  I wouldn't have thought
> that a use of REG should cause oprs_unchanged_p (REG, ..., false) to
> return false for source values involving REG.  It seems like this code
> is reusing reg_avail_info for what is logically a separate HARD_REG_SET.

Yes, that was not in the original implementation and was added when the pass 
was moved to its own file.  It's clearly a caching mechanism (see the head 
comment of reg_set_or_used_since_bb_start) but of course it overloads 
reg_avail_info (whose head comment has not been updated to reflect that).

> That said, why is reg_set_between_after_reload_p needed at all,
> given that we have already passed all insns through record_opr_changes:
>
> 	  /* Keep track of everything modified by this insn, so that we
> 	     know what has been modified since the start of the current
> 	     basic block.  */
> 	  if (INSN_P (insn))
> 	    record_opr_changes (insn);
>
> ?

Yes, I agree that there is definitely some redundancy here.  But it looks like 
reg_set_between_after_reload_p will additionally flag CLOBBERs for calls.

> Given that this is the only caller of reg_used_between_after_reload_p,
> and that the actual value of reg_avail_info doesn't matter in this
> context (its value is only used when testing oprs_unchanged_p (..., true)
> rather than oprs_unchanged_p (..., false)), I suppose that
> reg_used_between_after_reload_p doesn't need to check for
> call-clobberedness at all.  It will be covered by the
> reg_set_between_after_reload_p check or (if I'm right above)
> the first reg_avail_info check.

Yes, that would match reg_used_between_p, see above.

> So having written that, I suspect that the answer to your original
> question is that it's the reg_set_between_after_reload_p part that
> really matters, and it's the other caller:
>
> 	  if (! reg_set_between_after_reload_p (avail_reg, avail_insn,
> 						next_pred_bb_end))
> 	    /* AVAIL_INSN remains non-null.  */
> 	    break;
> 	  else
> 	    avail_insn = NULL;
>
> that was being led astray.

OK, that makes sense.

> I have a nasty suspicion I've talked myself into more invasive changes
> than the surface fix (which I still believe is correct as far as it goes).
> Problem is, I'm not sure I understand the intent of the original authors
> enough to rework the code in a more drastic way.  (It does all seem to
> be about implementation details rather than the textbook algorithm.)

Let's try to converge towards a reasonable cleanup.  Here's my proposal:

 1. The modification to reg_set_between_after_reload_p is OK provided that 
call_used_reg_set is replaced with regs_invalidated_by_call, as well as 
find_reg_fusage moved under the control of CALL_P.

 2. The same change is applied to record_opr_changes and the missing call to 
find_reg_fusage added.

 3. The same change is applied to reg_set_p in rtlanal.c.

 4. reg_used_between_after_reload_p is modified to use the same core test as 
reg_used_between_p (hence your original change is void).  As a consequence,
"Similar" is replaced with "Just like" in the head comment.

 5. reg_set_or_used_since_bb_start is modified not to clobber reg_avail_info 
anymore, only to test it.  The call to reg_set_between_after_reload_p is 
replaced with a comment explaining why it is redundant, hence useless.

What do you think?  I'll help you to test it.

-- 
Eric Botcazou

Re: Fix postreload-gcse treatment of call-clobbered registers

[Richard Sandiford]

Thanks for the detailed analysis.  Your plan sounded good to me.

Eric Botcazou <ebotcazou@libertysurf.fr> writes:
> Let's try to converge towards a reasonable cleanup.  Here's my proposal:
>
>  1. The modification to reg_set_between_after_reload_p is OK provided that 
> call_used_reg_set is replaced with regs_invalidated_by_call, as well as 
> find_reg_fusage moved under the control of CALL_P.

As far as I can tell, this makes reg_set_between_after_reload_p
identical to reg_set_between_p; the innards of both are equivalent to
reg_set_p.  And that's probably as it ought to be.  reg_set_between_p
should certainly provide accurate information for hard registers, and
nothing in the current reg_set_between_after_reload_p is really
optimised for non-pseudo usage.

>  2. The same change is applied to record_opr_changes and the missing call to 
> find_reg_fusage added.

Just so we're on the same page, I'm not sure what you mean by the
"same change", or by your earlier comment:

> Finally, record_opr_changes should also be modified along the lines of
> your change.

record_opr_changes already processes every hard register in
regs_invalidated_by_call, so I think that part is OK.

I've implemented the find_reg_fusage change in the patch below.

>  3. The same change is applied to reg_set_p in rtlanal.c.

OK, done.

>  4. reg_used_between_after_reload_p is modified to use the same core test as 
> reg_used_between_p (hence your original change is void).  As a consequence,
> "Similar" is replaced with "Just like" in the head comment.

As with the *_set_* functions, they are now so "just like" that I think
they're identical.  I've therefore removed the function and used
reg_used_between_p instead.

>  5. reg_set_or_used_since_bb_start is modified not to clobber reg_avail_info 
> anymore, only to test it.  The call to reg_set_between_after_reload_p is 
> replaced with a comment explaining why it is redundant, hence useless.

OK, done.  (As far as the comment goes, I added some preconditions
to the function comment.)

> What do you think?  I'll help you to test it.

I think we should also check the reg_avail_info entry for every
individual register, rather than just the first.  I've added a
new function (reg_changed_after_insn_p) to do that.

Does the new patch look OK?  Bootstrapped & regression-tested
on x86_64-linux-gnu.

Richard


gcc/
	* postreload-gcse.c (reg_changed_after_insn_p): New function.
	(oprs_unchanged_p): Use it to check all registers in a REG.
	(record_opr_changes): Look for clobbers in CALL_INSN_FUNCTION_USAGE.
	(reg_set_between_after_reload_p): Delete.
	(reg_used_between_after_reload_p): Likewise.
	(reg_set_or_used_since_bb_start): Use reg_changed_after_insn_p
	to check reg_avail_info for every individual hard register.
	Use reg_used_between_p instead of reg_used_between_after_reload_p.
	Do not call reg_set_between_after_reload_p.
	(eliminate_partially_redundant_load): Use reg_set_between_p
	instead of reg_set_between_after_reload_p.
	* rtlanal.c (reg_set_p): Check whether REG overlaps
	regs_invalidated_by_call, rather than just checking the
	membership of REGNO (REG).

Index: gcc/postreload-gcse.c
===================================================================
--- gcc/postreload-gcse.c	(revision 124984)
+++ gcc/postreload-gcse.c	(working copy)
@@ -197,8 +197,6 @@ static void dump_hash_table (FILE *);
 static bool reg_killed_on_edge (rtx, edge);
 static bool reg_used_on_edge (rtx, edge);
 
-static rtx reg_set_between_after_reload_p (rtx, rtx, rtx);
-static rtx reg_used_between_after_reload_p (rtx, rtx, rtx);
 static rtx get_avail_load_store_reg (rtx);
 
 static bool bb_has_well_behaved_predecessors (basic_block);
@@ -470,6 +468,22 @@ dump_hash_table (FILE *file)
   fprintf (file, "\n");
 }
 \f
+/* Return true if register X is recorded as being set by an instruction
+   whose CUID is greater than the one given.  */
+
+static bool
+reg_changed_after_insn_p (rtx x, int cuid)
+{
+  unsigned int regno, end_regno;
+
+  regno = REGNO (x);
+  end_regno = END_HARD_REGNO (x);
+  do
+    if (reg_avail_info[regno] > cuid)
+      return true;
+  while (++regno < end_regno);
+  return false;
+}
 
 /* Return nonzero if the operands of expression X are unchanged
    1) from the start of INSN's basic block up to but not including INSN
@@ -495,12 +509,12 @@ oprs_unchanged_p (rtx x, rtx insn, bool 
       if (after_insn)
 	/* If the last CUID setting the insn is less than the CUID of
 	   INSN, then reg X is not changed in or after INSN.  */
-	return reg_avail_info[REGNO (x)] < INSN_CUID (insn);
+	return !reg_changed_after_insn_p (x, INSN_CUID (insn) - 1);
       else
 	/* Reg X is not set before INSN in the current basic block if
 	   we have not yet recorded the CUID of an insn that touches
 	   the reg.  */
-	return reg_avail_info[REGNO (x)] == 0;
+	return !reg_changed_after_insn_p (x, 0);
 
     case MEM:
       if (load_killed_in_block_p (INSN_CUID (insn), x, after_insn))
@@ -717,12 +731,28 @@ record_opr_changes (rtx insn)
   /* Finally, if this is a call, record all call clobbers.  */
   if (CALL_P (insn))
     {
-      unsigned int regno;
+      unsigned int regno, end_regno;
+      rtx link, x;
 
       for (regno = 0; regno < FIRST_PSEUDO_REGISTER; regno++)
 	if (TEST_HARD_REG_BIT (regs_invalidated_by_call, regno))
 	  record_last_reg_set_info (insn, regno);
 
+      for (link = CALL_INSN_FUNCTION_USAGE (insn); link; link = XEXP (link, 1))
+	if (GET_CODE (XEXP (link, 0)) == CLOBBER)
+	  {
+	    x = XEXP (XEXP (link, 0), 0);
+	    if (REG_P (x))
+	      {
+		gcc_assert (HARD_REGISTER_P (x));
+	        regno = REGNO (x);
+		end_regno = END_HARD_REGNO (x);
+		do
+		  record_last_reg_set_info (insn, regno);
+		while (++regno < end_regno);
+	      }
+	  }
+
       if (! CONST_OR_PURE_CALL_P (insn))
 	record_last_mem_set_info (insn);
     }
@@ -857,93 +887,15 @@ reg_used_on_edge (rtx reg, edge e)
 }
 \f
 
-/* Return the insn that sets register REG or clobbers it in between
-   FROM_INSN and TO_INSN (exclusive of those two).
-   Just like reg_set_between but for hard registers and not pseudos.  */
-
-static rtx
-reg_set_between_after_reload_p (rtx reg, rtx from_insn, rtx to_insn)
-{
-  rtx insn;
-
-  /* We are called after register allocation.  */
-  gcc_assert (REG_P (reg) && REGNO (reg) < FIRST_PSEUDO_REGISTER);
-
-  if (from_insn == to_insn)
-    return NULL_RTX;
-
-  for (insn = NEXT_INSN (from_insn);
-       insn != to_insn;
-       insn = NEXT_INSN (insn))
-    if (INSN_P (insn))
-      {
-	if (set_of (reg, insn) != NULL_RTX)
-	  return insn;
-	if ((CALL_P (insn)
-	      && call_used_regs[REGNO (reg)])
-	    || find_reg_fusage (insn, CLOBBER, reg))
-	  return insn;
-
-	if (FIND_REG_INC_NOTE (insn, reg))
-	  return insn;
-      }
-
-  return NULL_RTX;
-}
-
-/* Return the insn that uses register REG in between FROM_INSN and TO_INSN
-   (exclusive of those two). Similar to reg_used_between but for hard
-   registers and not pseudos.  */
-
-static rtx
-reg_used_between_after_reload_p (rtx reg, rtx from_insn, rtx to_insn)
-{
-  rtx insn;
-
-  /* We are called after register allocation.  */
-  gcc_assert (REG_P (reg) && REGNO (reg) < FIRST_PSEUDO_REGISTER);
-
-  if (from_insn == to_insn)
-    return NULL_RTX;
-
-  for (insn = NEXT_INSN (from_insn);
-       insn != to_insn;
-       insn = NEXT_INSN (insn))
-    if (INSN_P (insn))
-      {
-	if (reg_overlap_mentioned_p (reg, PATTERN (insn))
-	    || (CALL_P (insn)
-		&& call_used_regs[REGNO (reg)])
-	    || find_reg_fusage (insn, USE, reg)
-	    || find_reg_fusage (insn, CLOBBER, reg))
-	  return insn;
-
-	if (FIND_REG_INC_NOTE (insn, reg))
-	  return insn;
-      }
-
-  return NULL_RTX;
-}
-
 /* Return true if REG is used, set, or killed between the beginning of
-   basic block BB and UP_TO_INSN.  Caches the result in reg_avail_info.  */
+   basic block BB and UP_TO_INSN.  Assume that reg_avail_info[R] is
+   nonzero if R is set during that time.  */
 
 static bool
 reg_set_or_used_since_bb_start (rtx reg, basic_block bb, rtx up_to_insn)
 {
-  rtx insn, start = PREV_INSN (BB_HEAD (bb));
-
-  if (reg_avail_info[REGNO (reg)] != 0)
-    return true;
-
-  insn = reg_used_between_after_reload_p (reg, start, up_to_insn);
-  if (! insn)
-    insn = reg_set_between_after_reload_p (reg, start, up_to_insn);
-
-  if (insn)
-    reg_avail_info[REGNO (reg)] = INSN_CUID (insn);
-
-  return insn != NULL_RTX;
+  return (reg_changed_after_insn_p (reg, 0)
+	  || reg_used_between_p (reg, PREV_INSN (BB_HEAD (bb)), up_to_insn));
 }
 
 /* Return the loaded/stored register of a load/store instruction.  */
@@ -1068,8 +1020,7 @@ eliminate_partially_redundant_load (basi
 	      avail_insn = NULL;
 	      continue;
 	    }
-	  if (! reg_set_between_after_reload_p (avail_reg, avail_insn,
-						next_pred_bb_end))
+	  if (!reg_set_between_p (avail_reg, avail_insn, next_pred_bb_end))
 	    /* AVAIL_INSN remains non-null.  */
 	    break;
 	  else
Index: gcc/rtlanal.c
===================================================================
--- gcc/rtlanal.c	(revision 124984)
+++ gcc/rtlanal.c	(working copy)
@@ -825,8 +825,8 @@ reg_set_p (rtx reg, rtx insn)
 	  || (CALL_P (insn)
 	      && ((REG_P (reg)
 		   && REGNO (reg) < FIRST_PSEUDO_REGISTER
-		   && TEST_HARD_REG_BIT (regs_invalidated_by_call,
-					 REGNO (reg)))
+		   && overlaps_hard_reg_set_p (regs_invalidated_by_call,
+					       GET_MODE (reg), REGNO (reg)))
 		  || MEM_P (reg)
 		  || find_reg_fusage (insn, CLOBBER, reg)))))
     return 1;

Re: Fix postreload-gcse treatment of call-clobbered registers

[Eric Botcazou]

> As far as I can tell, this makes reg_set_between_after_reload_p
> identical to reg_set_between_p; the innards of both are equivalent to
> reg_set_p.  And that's probably as it ought to be.  reg_set_between_p
> should certainly provide accurate information for hard registers, and
> nothing in the current reg_set_between_after_reload_p is really
> optimised for non-pseudo usage.

Even better!

> Just so we're on the same page, I'm not sure what you mean by the
>
> "same change", or by your earlier comment:
> > Finally, record_opr_changes should also be modified along the lines of
> > your change.
>
> record_opr_changes already processes every hard register in
> regs_invalidated_by_call, so I think that part is OK.

Ah, yes, I was confused.

> I've implemented the find_reg_fusage change in the patch below.

Thanks.

> I think we should also check the reg_avail_info entry for every
> individual register, rather than just the first.  I've added a
> new function (reg_changed_after_insn_p) to do that.

I think you're right.

> @@ -495,12 +509,12 @@ oprs_unchanged_p (rtx x, rtx insn, bool
>        if (after_insn)
>  	/* If the last CUID setting the insn is less than the CUID of
>  	   INSN, then reg X is not changed in or after INSN.  */
> -	return reg_avail_info[REGNO (x)] < INSN_CUID (insn);
> +	return !reg_changed_after_insn_p (x, INSN_CUID (insn) - 1);
>        else
>  	/* Reg X is not set before INSN in the current basic block if
>  	   we have not yet recorded the CUID of an insn that touches
>  	   the reg.  */
> -	return reg_avail_info[REGNO (x)] == 0;
> +	return !reg_changed_after_insn_p (x, 0);

I think that both comments are now outdated and should be removed, the logic 
is now fully encapsulated in reg_changed_after_insn_p.

>  static bool
>  reg_set_or_used_since_bb_start (rtx reg, basic_block bb, rtx up_to_insn)
>  {
> -  rtx insn, start = PREV_INSN (BB_HEAD (bb));
> -
> -  if (reg_avail_info[REGNO (reg)] != 0)
> -    return true;
> -
> -  insn = reg_used_between_after_reload_p (reg, start, up_to_insn);
> -  if (! insn)
> -    insn = reg_set_between_after_reload_p (reg, start, up_to_insn);
> -
> -  if (insn)
> -    reg_avail_info[REGNO (reg)] = INSN_CUID (insn);
> -
> -  return insn != NULL_RTX;
> +  return (reg_changed_after_insn_p (reg, 0)
> +	  || reg_used_between_p (reg, PREV_INSN (BB_HEAD (bb)), up_to_insn));

I think that reg_set_or_used_since_bb_start now serves no useful purpose and 
should be inlined back in its caller.

> 	* postreload-gcse.c (reg_changed_after_insn_p): New function.
> 	(oprs_unchanged_p): Use it to check all registers in a REG.
> 	(record_opr_changes): Look for clobbers in CALL_INSN_FUNCTION_USAGE.
> 	(reg_set_between_after_reload_p): Delete.
> 	(reg_used_between_after_reload_p): Likewise.
> 	(reg_set_or_used_since_bb_start): Use reg_changed_after_insn_p
> 	to check reg_avail_info for every individual hard register.
> 	Use reg_used_between_p instead of reg_used_between_after_reload_p.
> 	Do not call reg_set_between_after_reload_p.
> 	(eliminate_partially_redundant_load): Use reg_set_between_p
> 	instead of reg_set_between_after_reload_p.
> 	* rtlanal.c (reg_set_p): Check whether REG overlaps
> 	regs_invalidated_by_call, rather than just checking the
> 	membership of REGNO (REG).

OK if you remove the couple of comments and optionally inline r_g_o_u_s_b_s, 
in which case no need to retest either, I've bootstrapped/regtested this 
version with RTL checking for C/C++/Ada on x86, the important hunk being

@@ -1037,7 +972,8 @@ eliminate_partially_redundant_load (basi
 
   /* Check that the loaded register is not used, set, or killed from the
      beginning of the block.  */
-  if (reg_set_or_used_since_bb_start (dest, bb, insn))
+  if (reg_changed_after_insn_p (dest, 0)
+      || reg_used_between_p (dest, PREV_INSN (BB_HEAD (bb)), insn))
     return;

Thanks for your cooperation. ;-)

-- 
Eric Botcazou

Re: Fix postreload-gcse treatment of call-clobbered registers

[Richard Sandiford]

Eric Botcazou <ebotcazou@libertysurf.fr> writes:
>> @@ -495,12 +509,12 @@ oprs_unchanged_p (rtx x, rtx insn, bool
>>        if (after_insn)
>>  	/* If the last CUID setting the insn is less than the CUID of
>>  	   INSN, then reg X is not changed in or after INSN.  */
>> -	return reg_avail_info[REGNO (x)] < INSN_CUID (insn);
>> +	return !reg_changed_after_insn_p (x, INSN_CUID (insn) - 1);
>>        else
>>  	/* Reg X is not set before INSN in the current basic block if
>>  	   we have not yet recorded the CUID of an insn that touches
>>  	   the reg.  */
>> -	return reg_avail_info[REGNO (x)] == 0;
>> +	return !reg_changed_after_insn_p (x, 0);
>
> I think that both comments are now outdated and should be removed, the logic 
> is now fully encapsulated in reg_changed_after_insn_p.

Good point.  I removed them.

>>  static bool
>>  reg_set_or_used_since_bb_start (rtx reg, basic_block bb, rtx up_to_insn)
>>  {
>> -  rtx insn, start = PREV_INSN (BB_HEAD (bb));
>> -
>> -  if (reg_avail_info[REGNO (reg)] != 0)
>> -    return true;
>> -
>> -  insn = reg_used_between_after_reload_p (reg, start, up_to_insn);
>> -  if (! insn)
>> -    insn = reg_set_between_after_reload_p (reg, start, up_to_insn);
>> -
>> -  if (insn)
>> -    reg_avail_info[REGNO (reg)] = INSN_CUID (insn);
>> -
>> -  return insn != NULL_RTX;
>> +  return (reg_changed_after_insn_p (reg, 0)
>> +	  || reg_used_between_p (reg, PREV_INSN (BB_HEAD (bb)), up_to_insn));
>
> I think that reg_set_or_used_since_bb_start now serves no useful purpose and 
> should be inlined back in its caller.

Agreed.  The implementation is short and follows naturally from
the comment above the caller.

>> 	* postreload-gcse.c (reg_changed_after_insn_p): New function.
>> 	(oprs_unchanged_p): Use it to check all registers in a REG.
>> 	(record_opr_changes): Look for clobbers in CALL_INSN_FUNCTION_USAGE.
>> 	(reg_set_between_after_reload_p): Delete.
>> 	(reg_used_between_after_reload_p): Likewise.
>> 	(reg_set_or_used_since_bb_start): Use reg_changed_after_insn_p
>> 	to check reg_avail_info for every individual hard register.
>> 	Use reg_used_between_p instead of reg_used_between_after_reload_p.
>> 	Do not call reg_set_between_after_reload_p.
>> 	(eliminate_partially_redundant_load): Use reg_set_between_p
>> 	instead of reg_set_between_after_reload_p.
>> 	* rtlanal.c (reg_set_p): Check whether REG overlaps
>> 	regs_invalidated_by_call, rather than just checking the
>> 	membership of REGNO (REG).
>
> OK if you remove the couple of comments and optionally inline r_g_o_u_s_b_s, 
> in which case no need to retest either, I've bootstrapped/regtested this 
> version with RTL checking for C/C++/Ada on x86, the important hunk being
>
> @@ -1037,7 +972,8 @@ eliminate_partially_redundant_load (basi
>  
>    /* Check that the loaded register is not used, set, or killed from the
>       beginning of the block.  */
> -  if (reg_set_or_used_since_bb_start (dest, bb, insn))
> +  if (reg_changed_after_insn_p (dest, 0)
> +      || reg_used_between_p (dest, PREV_INSN (BB_HEAD (bb)), insn))
>      return;

Thanks for the review & testing.  Here's what I installed.

Richard


gcc/
	* postreload-gcse.c (reg_changed_after_insn_p): New function.
	(oprs_unchanged_p): Use it to check all registers in a REG.
	(record_opr_changes): Look for clobbers in CALL_INSN_FUNCTION_USAGE.
	(reg_set_between_after_reload_p): Delete.
	(reg_used_between_after_reload_p): Likewise.
	(reg_set_or_used_since_bb_start): Likewise.
	(eliminate_partially_redundant_load): Use reg_changed_after_insn_p
	and reg_used_between_p instead of reg_set_or_used_since_bb_start.
	Use reg_set_between_p instead of reg_set_between_after_reload_p.
	* rtlanal.c (reg_set_p): Check whether REG overlaps
	regs_invalidated_by_call, rather than just checking the
	membership of REGNO (REG).

Index: gcc/postreload-gcse.c
===================================================================
--- gcc/postreload-gcse.c	(revision 124984)
+++ gcc/postreload-gcse.c	(working copy)
@@ -197,8 +197,6 @@ static void dump_hash_table (FILE *);
 static bool reg_killed_on_edge (rtx, edge);
 static bool reg_used_on_edge (rtx, edge);
 
-static rtx reg_set_between_after_reload_p (rtx, rtx, rtx);
-static rtx reg_used_between_after_reload_p (rtx, rtx, rtx);
 static rtx get_avail_load_store_reg (rtx);
 
 static bool bb_has_well_behaved_predecessors (basic_block);
@@ -470,6 +468,22 @@ dump_hash_table (FILE *file)
   fprintf (file, "\n");
 }
 \f
+/* Return true if register X is recorded as being set by an instruction
+   whose CUID is greater than the one given.  */
+
+static bool
+reg_changed_after_insn_p (rtx x, int cuid)
+{
+  unsigned int regno, end_regno;
+
+  regno = REGNO (x);
+  end_regno = END_HARD_REGNO (x);
+  do
+    if (reg_avail_info[regno] > cuid)
+      return true;
+  while (++regno < end_regno);
+  return false;
+}
 
 /* Return nonzero if the operands of expression X are unchanged
    1) from the start of INSN's basic block up to but not including INSN
@@ -493,14 +507,9 @@ oprs_unchanged_p (rtx x, rtx insn, bool 
       /* We are called after register allocation.  */
       gcc_assert (REGNO (x) < FIRST_PSEUDO_REGISTER);
       if (after_insn)
-	/* If the last CUID setting the insn is less than the CUID of
-	   INSN, then reg X is not changed in or after INSN.  */
-	return reg_avail_info[REGNO (x)] < INSN_CUID (insn);
+	return !reg_changed_after_insn_p (x, INSN_CUID (insn) - 1);
       else
-	/* Reg X is not set before INSN in the current basic block if
-	   we have not yet recorded the CUID of an insn that touches
-	   the reg.  */
-	return reg_avail_info[REGNO (x)] == 0;
+	return !reg_changed_after_insn_p (x, 0);
 
     case MEM:
       if (load_killed_in_block_p (INSN_CUID (insn), x, after_insn))
@@ -717,12 +726,28 @@ record_opr_changes (rtx insn)
   /* Finally, if this is a call, record all call clobbers.  */
   if (CALL_P (insn))
     {
-      unsigned int regno;
+      unsigned int regno, end_regno;
+      rtx link, x;
 
       for (regno = 0; regno < FIRST_PSEUDO_REGISTER; regno++)
 	if (TEST_HARD_REG_BIT (regs_invalidated_by_call, regno))
 	  record_last_reg_set_info (insn, regno);
 
+      for (link = CALL_INSN_FUNCTION_USAGE (insn); link; link = XEXP (link, 1))
+	if (GET_CODE (XEXP (link, 0)) == CLOBBER)
+	  {
+	    x = XEXP (XEXP (link, 0), 0);
+	    if (REG_P (x))
+	      {
+		gcc_assert (HARD_REGISTER_P (x));
+	        regno = REGNO (x);
+		end_regno = END_HARD_REGNO (x);
+		do
+		  record_last_reg_set_info (insn, regno);
+		while (++regno < end_regno);
+	      }
+	  }
+
       if (! CONST_OR_PURE_CALL_P (insn))
 	record_last_mem_set_info (insn);
     }
@@ -856,96 +881,6 @@ reg_used_on_edge (rtx reg, edge e)
   return false;
 }
 \f
-
-/* Return the insn that sets register REG or clobbers it in between
-   FROM_INSN and TO_INSN (exclusive of those two).
-   Just like reg_set_between but for hard registers and not pseudos.  */
-
-static rtx
-reg_set_between_after_reload_p (rtx reg, rtx from_insn, rtx to_insn)
-{
-  rtx insn;
-
-  /* We are called after register allocation.  */
-  gcc_assert (REG_P (reg) && REGNO (reg) < FIRST_PSEUDO_REGISTER);
-
-  if (from_insn == to_insn)
-    return NULL_RTX;
-
-  for (insn = NEXT_INSN (from_insn);
-       insn != to_insn;
-       insn = NEXT_INSN (insn))
-    if (INSN_P (insn))
-      {
-	if (set_of (reg, insn) != NULL_RTX)
-	  return insn;
-	if ((CALL_P (insn)
-	      && call_used_regs[REGNO (reg)])
-	    || find_reg_fusage (insn, CLOBBER, reg))
-	  return insn;
-
-	if (FIND_REG_INC_NOTE (insn, reg))
-	  return insn;
-      }
-
-  return NULL_RTX;
-}
-
-/* Return the insn that uses register REG in between FROM_INSN and TO_INSN
-   (exclusive of those two). Similar to reg_used_between but for hard
-   registers and not pseudos.  */
-
-static rtx
-reg_used_between_after_reload_p (rtx reg, rtx from_insn, rtx to_insn)
-{
-  rtx insn;
-
-  /* We are called after register allocation.  */
-  gcc_assert (REG_P (reg) && REGNO (reg) < FIRST_PSEUDO_REGISTER);
-
-  if (from_insn == to_insn)
-    return NULL_RTX;
-
-  for (insn = NEXT_INSN (from_insn);
-       insn != to_insn;
-       insn = NEXT_INSN (insn))
-    if (INSN_P (insn))
-      {
-	if (reg_overlap_mentioned_p (reg, PATTERN (insn))
-	    || (CALL_P (insn)
-		&& call_used_regs[REGNO (reg)])
-	    || find_reg_fusage (insn, USE, reg)
-	    || find_reg_fusage (insn, CLOBBER, reg))
-	  return insn;
-
-	if (FIND_REG_INC_NOTE (insn, reg))
-	  return insn;
-      }
-
-  return NULL_RTX;
-}
-
-/* Return true if REG is used, set, or killed between the beginning of
-   basic block BB and UP_TO_INSN.  Caches the result in reg_avail_info.  */
-
-static bool
-reg_set_or_used_since_bb_start (rtx reg, basic_block bb, rtx up_to_insn)
-{
-  rtx insn, start = PREV_INSN (BB_HEAD (bb));
-
-  if (reg_avail_info[REGNO (reg)] != 0)
-    return true;
-
-  insn = reg_used_between_after_reload_p (reg, start, up_to_insn);
-  if (! insn)
-    insn = reg_set_between_after_reload_p (reg, start, up_to_insn);
-
-  if (insn)
-    reg_avail_info[REGNO (reg)] = INSN_CUID (insn);
-
-  return insn != NULL_RTX;
-}
-
 /* Return the loaded/stored register of a load/store instruction.  */
 
 static rtx
@@ -1037,7 +972,8 @@ eliminate_partially_redundant_load (basi
 
   /* Check that the loaded register is not used, set, or killed from the
      beginning of the block.  */
-  if (reg_set_or_used_since_bb_start (dest, bb, insn))
+  if (reg_changed_after_insn_p (dest, 0)
+      || reg_used_between_p (dest, PREV_INSN (BB_HEAD (bb)), insn))
     return;
 
   /* Check potential for replacing load with copy for predecessors.  */
@@ -1068,8 +1004,7 @@ eliminate_partially_redundant_load (basi
 	      avail_insn = NULL;
 	      continue;
 	    }
-	  if (! reg_set_between_after_reload_p (avail_reg, avail_insn,
-						next_pred_bb_end))
+	  if (!reg_set_between_p (avail_reg, avail_insn, next_pred_bb_end))
 	    /* AVAIL_INSN remains non-null.  */
 	    break;
 	  else
Index: gcc/rtlanal.c
===================================================================
--- gcc/rtlanal.c	(revision 124984)
+++ gcc/rtlanal.c	(working copy)
@@ -825,8 +825,8 @@ reg_set_p (rtx reg, rtx insn)
 	  || (CALL_P (insn)
 	      && ((REG_P (reg)
 		   && REGNO (reg) < FIRST_PSEUDO_REGISTER
-		   && TEST_HARD_REG_BIT (regs_invalidated_by_call,
-					 REGNO (reg)))
+		   && overlaps_hard_reg_set_p (regs_invalidated_by_call,
+					       GET_MODE (reg), REGNO (reg)))
 		  || MEM_P (reg)
 		  || find_reg_fusage (insn, CLOBBER, reg)))))
     return 1;

Re: Fix postreload-gcse treatment of call-clobbered registers

[Ian Lance Taylor]

Richard Sandiford <rsandifo@nildram.co.uk> writes:

> This is the patch that inspired the HARD_REG_SET constructs I just posted
> (and it depends on them).  postreload-gcse.c was only checking whether
> REGNO (x) is call-clobbered, but it should check whether the whole
> register is.

By the way, I think there is a similar case in local-alloc.c, though
it is an optimization issue rather than a correctness one:

		  /* Avoid making a call-saved register unnecessarily
                     clobbered.  */
		  hard_reg = get_hard_reg_initial_reg (cfun, r1);
		  if (hard_reg != NULL_RTX)
		    {
		      if (REG_P (hard_reg)
			  && REGNO (hard_reg) < FIRST_PSEUDO_REGISTER
			  && !call_used_regs[REGNO (hard_reg)])
			continue;
		    }

As far as I can tell r1 may occupy multiple registers, but this code
only checks the first one.

Ian