This is the mail archive of the 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

> 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

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