[RTL, ColdFire 24/63] Add support for a MODE_INDEX_REG_CLASS macro

Richard Sandiford richard@codesourcery.com
Wed Feb 28 16:17:00 GMT 2007


Jeffrey Law <law@redhat.com> writes:
> On Tue, 2007-02-27 at 21:14 +0000, Richard Sandiford wrote:
>> I disagree.  BASE_REG_CLASS is a guarantee from the backend that
>> any register in that class is a valid base register.  Reload relies
>> on that fact.  Why should INDEX_REG_CLASS by any different?
> Then we disagree.  I've explicitly rejected the patch that adds the mode
> dependent index register class stuff.  I believe it is unnecessary and
> wrong based on working with GCC for more than 15 years, including on
> targets which have similar characteristics as the coldfire.
>
> I've pointed you at a more correct fix.  You can pursue that or some
> other approach.

OK, I've tested your patch, and it seems to work fine on ColdFire.
I don't think it's correct to use GET_MODE_SIZE as the offset though,
because GET_MODE_SIZE is 0 for things like BLKmode.  I think the patch
would break the elimination of BLKmode addresses on targets like PowerPC
that have base+index but not disp+base+index addresses.

My concern about efficiency still stands though.  Concretely,
your patch is going to mean that the MIPS port does (29 modes x
188 regs =) 5452 legitimate address checks at startup, all of
which will be false.  The figure for PowerPC is (38 modes x 114 regs =)
4332 checks and the figure for ia64 is (40 modes * 334 regs =) 13360 checks.
(I realise that ia64 is one of those targets where you expect the compiler
to be slower, but still. ;))  As the patch stands, it will also create
new garbage addresses for each check.  I think at the very least you
should:

  - create one address and use it for all checks, changing parts of
    it as appropriate

  - restrict the register loop to members of INDEX_REG_CLASS

...and perhaps skip the loop entirely if INDEX_REG_CLASS is NO_REGS.

I notice that you've kept the INDEX_REG_CLASS != NO_REGS check in
postreload.c.  Given your position on this, do you think that that
check is really necessary?

I haven't tried to update the patch along these lines because
I still want to pitch the original patch as being the correct
fix for this problem.  Please bear me out here; it comes back
to the double_reg_address_ok thing, and I think it provides a
neater (more correct) fix for it.

My position has been that INDEX_REG_CLASS should be treated as a
guarantee that any register of that class is valid as an index
register.  It seems to me that the relationship between G_I_L_A
and *_REG_CLASS is very similar to the relationship between
predicates and constraints.  On a well-tuned port -- especially
one like PowerPC where the predicates enforce class restrictions
on hard registers -- the predicates answer the question "is this
operand valid?" (or, on less-well-tuned targets, "is this operand
reloadable?"), whereas the constraints say "how do I make this
operand valid?".  I know that's an over-simplification, but I think
it is basically true.  And it seems to me that G_I_L_A plays the same
role as the predicates for addresses -- "is this address valid?",
or "is this address reloadable?"[*] -- while *_REG_CLASS-related
macros answer the question "how do I make this address valid?".
Now it's clearly wrong for an insn's constraints to say X is valid
if the predicates reject X, and I don't think we'd consider a patch
that was specifically designed to allow such constraints to be used.
I think the same should be true of the *_REG_CLASS-related macros.

In the code guarded by double_reg_address_ok, reload is trying to create
a valid offsetable base+index address.  Given the above, I think the
*_REG_CLASS group of macros -- the "how do I make this address valid?"
macros -- ought to be directly telling reload what class of index
register is allowed in such an address.  And the backend should answer
NO_REGS if it doesn't allow any register to be used like that,
much as EXTRA_CONSTRAINT would return NO_REGS for register classes
that aren't available with the current command-line options.
An obvious way do to this would be add an "offsetable_p" argument to
MODE_INDEX_REG_CLASS and mode_reg_class (i.e. the addresses.h function
in my patch that wraps the target macros).

This approach seems far cleaner to me.  The current code, and the
code after your patch, is jumping through hoops trying to guess what
constitutes an offsetable base+index address, and in which contexts
such an address is valid.  But this is information that the backend knows
a priori.  I think adding an "offsetable_p" argument to MODE_INDEX_REG_CLASS
would be:

  - more correct (from my view of the world), because it tallies with
    the division of labour described above.

  - more efficient.  We would have no need for double_reg_address_ok
    or its associated start-up code.  And in practice, the offsetable_p
    condition will be eliminated at compile time; the caller will always
    provide a constant value, and the definitions of the backend macros
    and mode_index_regs will be trivially foldable.

  - more forward-looking.  I think this is exactly the kind of
    information that the fabled define_address .md construct
    would provide.  In other words, I think the reload side of
    the define_address code would directly query the backend
    in this case, just as my proposal would.

Richard

[*] ...which isn't necessarily the same thing.  E.g. PowerPC (again)
    deliberately allows out-of-range offsets on eliminable registers,
    with the knowledge that reload will fix them up if the final
    register + offset is out of range.



More information about the Gcc-patches mailing list