On Mon, Nov 28, 2011 at 11:10:56PM +0100, Eric Botcazou wrote:
> This looks reasonable, but the logic is a bit hard to follow, especially the
> double usage of internal_arg_pointer_based_reg depending on SCAN's value.
> Would it be possible to split it into 2 functions that recursively call each
> other?
Well, splitting off the scanning into separate function is quite easy, but
that would still mean SCAN parameter. The thing is, for the common case
where ADDR is already internal_arg_pointer, or sum of that and CONST_INT, or
SYMBOL_REF, I'd prefer to avoid the scanning of the instructions and only do
that if really needed (i.e. when either looking for a pseudo or arbitrary
expression that needs to be for_each_rtx scanned). To get rid of the SCAN
argument I'm afraid the first part of the function would need to be
duplicated (for the current SCAN and !SCAN cases), then the scanning phase
in another function and the last part in another one.
> You should also make it clearer that internal_arg_pointer_seq_start is part
> of
> the caching mechanism (I wondered for some minutes whether it has anything to
> do with RTL sequences), maybe:
Ok.
> > +static int
> > +internal_arg_pointer_based_reg_1 (rtx *loc, void *data ATTRIBUTE_UNUSED)
> > +{
> > + if (REG_P (*loc) && internal_arg_pointer_based_reg (*loc, false) !=
> > NULL_RTX) + return 1;
> > + if (MEM_P (*loc))
> > + return -1;
> > + return 0;
> > +}
>
> Missing comment for the MEM_P case.
Will add.
>
> if (REG_P (reg) && HARD_REGISTER_P (reg))
> return NULL_RTX;
Ok. BTW, it fixes also wrong-code PR51323, so I'll be including new
testcase in the next patch iteration.
Jakub
