This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
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