This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: PATCH for the mainline.
Richard Sandiford <richard@codesourcery.com> writes:
> > Ian Lance Taylor wrote:
> >> To clarify a bit, there is code in combine to change the mode of a
> >> pseudo-register, when profitable, if the register is set only once;
> >> see can_change_dest_mode. However the test in can_change_dest_mode
> >> does not consider that the pseudo-register might appear in REG_EQUAL
> >> notes. There is an attempt to fix up that case in distribute_notes,
> >> but it doesn't catch all possibilities. Most obviously it doesn't
> >> catch the case where the insn with the note is not combined with
> >> anything, and hence the notes are never passed to distribute_notes.
> >>
> >> The bug can be seen by looking at debugging dumps. After combine the
> >> same pseudo-register appears with one mode in the insn and with
> >> another mode in a REG_EQUAL note.
> >>
> >> We keep a canonical version of each pseudo-register in the
> >> regno_reg_rtx array, so it is easy to update every use of the
> >> pseudo-register by updating that array. The old code used to simply
> >> replace the entry in the array, which of course did not affect other
> >> existing uses. This patch changes it to directly modify the entry in
> >> the array, by slightly extending the existing combine undo mechanism.
>
> You've probably already considered this, sorry, but is it really safe to
> change modes in notes like this? What if the REG_EQUAL note was a more
> complicated expression, like
>
> (plus:SI (reg:SI foo) (reg:SI bar))
>
> If you change the mode of foo, you get a noncanonical rtx. Is there
> something that stops the optimisation from triggering on notes like this,
> but which allows it for bare-register notes?
No, there isn't. My thinking was:
1) Previously we would wind referring to the same pseudo-register in
two different modes, which is also invalid RTL. Therefore this
change is not worse.
2) We already generate invalid RTL in REG_EQUAL notes; see, e.g.,
ix86_expand_setcc, which generates a QImode REG_EQUAL note with
operands that need not be QImode. Therefore this change should not
introduce any new problems.
3) Since the REG_EQUAL note is not going to be a constant, it will
only be used in cases where it can be recognized. Invalid RTL
won't be recognized. Therefore invalid RTL in REG_EQUAL notes
should be harmless.
4) I don't see any simple way to change can_change_dest_mode to
efficiently detect the case of a register which appears in a
REG_EQUAL note.
5) Most times we use a scratch register the result does not match
anyhow. That is, the effort is usually wasted. The old code
generated RTL which most times immediately becomes garbage. The
new code does not. Therefore, even if we fix can_change_dest_mode
to prevent changing the mode of a register which appears in a
REG_EQUAL note, the new code is still a beneficial change.
> Also, does this lead to situations in which the REG_EQUAL note has
> a different (non-VOID) mode from the set register? If so, what's the
> current thinking on whether that should be allowed? It always seems
> somewhat bogus to me to have a DImode register with an SImode note.
This can indeed lead to such situations. As you say, such situations
can already arise. They are currently harmless.
Ian