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: 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


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