This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]