Re: [PATCH] New MIPS interrupt handler patch

[Richard Sandiford <rdsandiford at googlemail dot com>]

Hi Chao-Ying,

Looking very good now, thanks.

"Fu, Chao-Ying" <fu@mips.com> writes:
>   One remaining issue is that the assembler complains about
> "used $at without ".set noat", because we access $1.
> How do we avoid it?

Oh yeah, I was going to ask about that.

We could either do something in FINAL_PRESCAN_INSN (and add a new
FINAL_POSTSCAN_INSN counterpart) or do something special in
mips_output_move.  I kind-of prefer the first.

Let's handle this separately from the main patch though.

> Index: gcc4x/gcc/gcc/config/mips/mips.c
> ===================================================================
> --- gcc4x.orig/gcc/gcc/config/mips/mips.c	2009-03-18 14:37:55.874689000 -0700
> +++ gcc4x/gcc/gcc/config/mips/mips.c	2009-03-18 18:23:14.638972000 -0700
> @@ -261,18 +261,28 @@ struct mips_frame_info GTY(()) {
>    /* Likewise FPR X.  */
>    unsigned int fmask;
>  
> -  /* The number of GPRs and FPRs saved.  */
> +  /* Likewise accumulator X.  */

Given the bug you found, let's tighten this up by saying:

  /* Likewise doubleword accumulator X ($acX).  */

> +  /* The number of GPRs, FPRs, accumulators and COP0 registers saved.  */

and:

  /* The number of GPRs, FPRs, doubleword accumulators and COP0
     registers saved.  */

> +  /* True if we want to keep interrupts masked in interrupt handlers.  */
> +  bool keep_interrupts_masked_p;
> +
> +  /* True if we want to use debug exception return in interrupt handlers.  */
> +  bool use_debug_exception_return_p;

More comment nitpicking, but:

  /* True if this is an interrupt handler that should keep interrupts
     masked.  */

  /* True if this is an interrupt handler that should use DERET
     instead of ERET.  */

> +/* Return true if REGNO is a register that is ordinarily call-clobbered
> +   but must nevertheless be preserved by an interrupt handler.  */
> +
> +static bool
> +mips_interrupt_extra_call_saved_reg_p (unsigned int regno)
> +{
> +  if (MD_REG_P (regno))
> +    return true;
> +
> +  if (TARGET_DSP && DSP_ACC_REG_P (regno))
> +    return true;
> +
> +  if (GP_REG_P (regno) && !cfun->machine->use_shadow_register_set_p)
> +    {
> +      /* $0 is hard-wired.  */
> +      if (regno == GP_REG_FIRST)
> +	return false;
> +
> +      /* The interrupt handler can treat kernel registers as
> +	 scratch registers.  */
> +      if (KERNEL_REG_P (regno))
> +	return false;
> +
> +      /* The function will return the stack pointer to its original value
> +	 anyway.  */
> +      if (regno == STACK_POINTER_REGNUM)
> +	return false;
> +
> +      /* Otherwise, all GPRs are call-clobbered.  */
> +      return true;

I realised later that this final "return true;" should be:

  return call_really_used_regs[regno];

That is, this function is supposed to return true for registers
that aren't ordinarily call-clobbered.

(My fault, sorry.)  

> +  /* Move above the accumulator save area.  */
> +  if (frame->num_acc > 0)
> +    {
> +      /* Each accumulator needs 2 word.  */

s/2 word/2 words/

> +	  /* Restore the original Status.  */
> +	  mem = gen_frame_mem (word_mode,
> +				 plus_constant (stack_pointer_rtx, offset));

Too much indentation on the last line; "plus_constant" should line up
with "word_mode".

> +  /* If the register is part of the GPRs that's saved for interrupt context,
> +     we need to mark it as used by the epilogue.  */

  /* An interrupt handler must preserve some registers that are
     ordinarily call-clobbered.  */

> +#define EPILOGUE_USES(REGNO)	(mips_epilogue_uses (REGNO))

#define EPILOGUE_USES(REGNO)	mips_epilogue_uses (REGNO)

> +Note, for the MIPS, you can specify the behavior of @code{interrupt} by
> +adding more attributes in addition to the interrupt attribute.
> +These attributes are @code{use_shadow_register_set},
> +@code{keep_interrupts_masked}, and @code{use_debug_exception_return}.
> +Without these attributes, the default behavior is as follows:
> +don't use shadow register set (use normal registers);
> +don't keep interrupts masked (enable nested interrupts);
> +don't use debug exception return instruction (use exception return instruction).
> +
> +@smallexample
> +void __attribute__ ((interrupt)) v0 ();
> +void __attribute__ ((interrupt, use_shadow_register_set)) v1 ();
> +void __attribute__ ((interrupt, keep_interrupts_masked)) v2 ();
> +void __attribute__ ((interrupt, use_debug_exception_return)) v3 ();
> +void __attribute__ ((interrupt, use_shadow_register_set,
> +		     keep_interrupts_masked)) v4 ();
> +void __attribute__ ((interrupt, use_shadow_register_set,
> +		     use_debug_exception_return)) v5 ();
> +void __attribute__ ((interrupt, keep_interrupts_masked,
> +		     use_debug_exception_return)) v6 ();
> +void __attribute__ ((interrupt, use_shadow_register_set,
> +		     keep_interrupts_masked,
> +		     use_debug_exception_return)) v7 ();
> +@end smallexample
> +

Maybe something like the following would be better:

----------------------------------------------------------------------------
On MIPS targets, you can use the following attributes to modify the behavior
of an interrupt handler:
@table @code
@item use_shadow_register_set
@cindex @code{use_shadow_register_set} attribute
Assume that the handler uses a shadow register set, instead of
the main general-purpose registers.

@item keep_interrupts_masked
@cindex @code{keep_interrupts_masked} attribute
Keep interrupts masked for the whole function.  Without this attribute,
GCC tries to reenable interrupts for as much of the function as it can.

@item use_debug_exception_return}
@cindex @code{use_debug_exception_return} attribute
Return using the @code{deret} instruction.  Interrupt handlers that don't
have this attribute return using @code{eret} instead.
@end @table

You can use any combination of these attributes, as shown below:
@smallexample
void __attribute__ ((interrupt)) v0 ();
void __attribute__ ((interrupt, use_shadow_register_set)) v1 ();
void __attribute__ ((interrupt, keep_interrupts_masked)) v2 ();
void __attribute__ ((interrupt, use_debug_exception_return)) v3 ();
void __attribute__ ((interrupt, use_shadow_register_set,
		     keep_interrupts_masked)) v4 ();
void __attribute__ ((interrupt, use_shadow_register_set,
		     use_debug_exception_return)) v5 ();
void __attribute__ ((interrupt, keep_interrupts_masked,
		     use_debug_exception_return)) v6 ();
void __attribute__ ((interrupt, use_shadow_register_set,
		     keep_interrupts_masked,
		     use_debug_exception_return)) v7 ();
@end smallexample
----------------------------------------------------------------------------

(completely untested; please check the info and pdf output to make
sure it looks OK.)

I'm not sure how good that is though.  Other suggestions welcome.

> -#define MIPS_EPILOGUE_TEMP_REGNUM (GP_REG_FIRST + 5)
> +#define MIPS_EPILOGUE_TEMP_REGNUM \
> +  (cfun->machine->interrupt_handler_p ?	K0_REG_NUM : GP_REG_FIRST + 5)

Should be a space after "?", not a tab.

OK for 4.5 with those changes, thanks.  (I don't think it's appropriate
for trunk stage 4, even though it's target-specific.)

Thanks for your patience and perseverence with this patch.

Richard