Re: RFA: 4.2 reload infrastructure to fix PR target/21623

[Bernd Schmidt <bernds_cb1 at t-online dot de>]

Joern RENNECKE wrote:
> Before deciding on how to best install an interim patch for 4.1, I would 
> like
> us to agree on the interface we are going to use for 4.2 - at least 
> inasmuch as
> it would matter for portability of code from the interim patch to the 
> permanent
> solution.
> Is the attached patch OK for 4.2 once we get into phase 1?

Thanks for tackling this problem.  Couple of comments on the patch - on 
the whole I think this is looking pretty good.

> +enum reg_class
> +secondary_reload_class (int in_p, enum reg_class class,
> +			enum machine_mode mode, rtx x)

Needs a comment before the function.  I think we're using bools now 
instead of ints for the first argument.

> +{
> +  enum insn_code icode;
> +  const char *insn_constraint;
> +  char insn_letter;
> +  secondary_reload_info sri;
> +
> +  sri.icode = CODE_FOR_nothing;
> +  class = targetm.secondary_reload (in_p, x, class, mode, &sri);

Maybe add a new variable "secondary_class" to hold the return value to 
avoid confusion.

> +      insn_constraint = insn_data[(int) icode].operand[!in_p].constraint;
> +      insn_constraint = insn_data[(int) icode].operand[2].constraint;

Likewise; this would be easier to understand if there were two 
variables, scratch_constraint and intermediate_constraint.  It would 
also be nice to have scratch_class and intermediate_class, of which we'd 
then pick one for secondary_class and if necessary one for 
tertiary_class.  As is, the code is quite hard to follow (what's 
"class"?  what's "insn_class"?)

> +#elif defined(HAVE_SECONDARY_RELOADS)
> +#ifndef SECONDARY_INPUT_RELOAD_CLASS
> +#define SECONDARY_INPUT_RELOAD_CLASS(CLASS, MODE, X) NO_REGS
> +#endif
> +#ifndef SECONDARY_OUTPUT_RELOAD_CLASS
> +#define SECONDARY_OUTPUT_RELOAD_CLASS(CLASS, MODE, X) NO_REGS
> +#endif
>  #endif

This part I'm not sure about - why do we need HAVE_SECONDARY_RELOADS, 
and why do we have to define SECONDARY_*_RELOAD_CLASS if we do have it? 
 Can't we just hide all that in the default_secondary_reload_class hook?

>  struct stdarg_info;
> +/* The struct used by the secondary_reload target hook.  */

Empty line after struct stdarg_info.

> +typedef struct secondary_reload_info
> +{
> +  enum machine_mode icode;

enum insn_code, surely.  Also, not much point having a struct for one 
member - any intentions to extend it?

> +} secondary_reload_info;
> +

Not sure I agree with the comments about recursion.  Isn't there a 
mismatch between what targetm.secondary_reload_class wants (an rtx) and 
what we can give it (an intermediate class and mode)?  Does the sh need 
multiple intermediate registers for any of its secondary reloads?  If 
so, it could be handled with additional scratches (point about 
inheritance taken, but is it going to be anything but a rare case?)


Ehhh.  Just spotted the following comment we have in regclass:
  /* If we need a secondary reload (we assume here that we are using
     the secondary reload as an intermediate, not a scratch register),

That's lame.  We should fix that at some point.

> Another question: what do you think we should do about the secondary_reload
> name clashes that Kaz has found?  Although it generates more code churn, I
> am inclined to rename all the target port instances of this function
> to <arch>_secondary_reload.

Agreed.


Bernd