Re: [PATCH] MIPS function attributes for interrupt handlers

[Richard Sandiford <rdsandiford at googlemail dot com>]

Hi Chao-Ying,

Thanks for the patch.  I'm afraid it's too invasive for stage 3,
but I'll review it with 4.5 in mind.

First of all, I really don't want to see the "naked" attribute spread
to new ports.  I think it's generally considered to have been a mistake
on the ports that do support it.  The whole point of the attribute is
to allow asms that wouldn't normally be allowed (in the sense that the
asms can make assumptions that wouldn't normally hold).  But the semantics
are muddy from a language point of view.

If someone wants a free-form asm function body with no
compiler-generated code, they really should write it in assembly.
They can use preprocessor macros to prettify the function declarations
if need be.  (This is of course what MIPS asm coders already do.)

"Fu, Chao-Ying" <fu@mips.com> writes:
> 2008-10-14  Chao-ying Fu  <fu@mips.com>
> 		James Grosbach <james.grosbach@microchip.com>

Does James have a copyright assignment on file?  We couldn't accept
the file without it.

> +  /* The number of interrupt reg saved.  */
> +  unsigned int num_interrupt_reg;

s/reg/regs/.  Might as well combine it with the GPR and FPR stuff,
like you did with the offsets:

  /* The number of GPRs, FPRs and interrupt registers saved.  */
  unsigned int num_gp;
  unsigned int num_fp;
  unsigned int num_interrupt_regs;

> +  HOST_WIDE_INT int_save_offset;
> [...]
> +  HOST_WIDE_INT int_sp_offset;

s/int/interrupt/ (here and elsewhere).  Sometimes you see GPRs referred
to as "int(eger)" registers, so "int" is a bit ambiguous.

> +  /* True if interrupt context is saved.  */
> +  bool has_interrupt_context_p;

This seems to be equivalent to "num_interrupt_reg(s) > 0"
(which is also used in the patch) or, after the change
suggested later, context_save_type != CONTEXT_SAVE_NONE.
I'd prefer not to have this field as well.

> +
> +  /* True if hi and lo registers are saved.  Will only be true for
> +     interrupts.  Need to take care of 4 hi-lo pairs.  */
> +  bool has_hilo_context_p[4];

This logically goes in the same block as the GPR and FPR masks.
The associated code is:

> +      /* If HI/LO is defined in this function, we need to save them too.
> +         If the function is not a leaf function, we assume that the
> +         called function uses them.  */
> +      if (!current_function_is_leaf || crtl->saves_all_registers)
> +	{
> +	  frame->num_interrupt_reg += 2;
> +          frame->has_hilo_context_p[0] = true;
> +	  if (TARGET_DSP)
> +	    {
> +	      frame->num_interrupt_reg += 6;
> +	      frame->has_hilo_context_p[1] = true;
> +	      frame->has_hilo_context_p[2] = true;
> +	      frame->has_hilo_context_p[3] = true;
> +	    }
> +	}
> +      else
> +	{
> +	  unsigned int i;
> +	  if (df_regs_ever_live_p (LO_REGNUM)
> +	      || df_regs_ever_live_p (HI_REGNUM))
> +	    {
> +	      frame->num_interrupt_reg += 2;
> +	      frame->has_hilo_context_p[0] = true;
> +	    }
> +
> +	  for (i = 2; i < 8; i += 2)
> +	    {
> +	      if (df_regs_ever_live_p (DSP_ACC_REG_FIRST + i - 2)
> +		  || df_regs_ever_live_p (DSP_ACC_REG_FIRST + i - 1))
> +	      {
> +		frame->num_interrupt_reg += 2;
> +		frame->has_hilo_context_p[i >> 1] = true;
> +	      }
> +	    }
> +	}
> +    }

I think it'd be better to handle this with mips_save_reg_p.
The logic above is basically duplicating the logic you added
to mips_save_reg_p, and it's too subtle to copy like this.

Also, for consistency, please use a mask instead of a boolean array.

I also don't think the accumulators should be counted as "interrupt"
registers.  Instead, we should have four save/restore groups:

  GPRs
  FPRs
  accumulators
  cp0 registers

(I'm not saying you should use a mask for the cp0 registers.
Only the accumulators need one of those.  But there should be
four separate register counts.)

> +  /* Move above the interrupt registers save area.  */
> +  if (frame->num_interrupt_reg > 0)
> +    {
> +      offset += MIPS_STACK_ALIGN (frame->num_interrupt_reg * UNITS_PER_WORD);
> +      frame->int_sp_offset = offset - UNITS_PER_WORD;
> +    }

Do we really need to align this area separately from the GPRs?

> +/* Is the current function an interrupt? If so, how is context handled?  */
> +enum function_type_tag {
> +  DONT_KNOW,
> +  NON_INTERRUPT,
> +  SOFTWARE_CONTEXT_SAVE,
> +  SRS_CONTEXT_SAVE
> +};
> +enum function_type_tag current_function_type = DONT_KNOW;

This needs more documentation, with a comment to say what each
value means.  Also, I'd prefer to have enum constants with a common
prefix, like we have for SYMBOL_, ADDRESS_, etc.  Maybe:

  INTERRUPT_CONTEXT_NONE
  INTERRUPT_CONTEXT_MAIN_REGS
  INTERRUPT_CONTEXT_SHADOW_REGS

?

current_function_type should be part of machine_function, where
"interrupt_context_type" might be a better name.  There'd then be no
need for DONT_KNOW.  (Although I agree it's correct to cache the value
across calls to mips_compute_frame_info, there's no real need to.
mips_compute_frame_info conceptually recomputes everything from
scratch and isn't called very often.)

> +/* MIPS devices also allow interrupt vector dispactch functions
> +   to be defined via an attribute (of the interrupt handler).
> +   We keep a list of the dispatch functions needed and emit them at
> +   the end of processing the translation unit.  */

s/MIPS devices also allow/We allow/ (unless I'm misunderstanding
what the comment is trying to say)

> +struct vector_dispatch_spec
> +{
> +  const char *target;	/* Target function name.  */
> +  int vector_number;	/* Exception vector table number.  */
> +  struct vector_dispatch_spec *next;
> +};

Comments should go above the field.

> +struct vector_dispatch_spec *vector_dispatch_list_head;

Should be static.  I slightly prefer "vector_dispatch_list".

> +/* Check if the interrupt attribute is set for a function. If it is, return
> +   the IPL identifier, else NULL.  */
> +
> +static tree
> +mips_interrupt_type_p (tree type)
> +{
> +  tree attr = lookup_attribute ("interrupt", TYPE_ATTRIBUTES (type));
> +  if (attr)
> +    attr = TREE_VALUE (attr);
> +  return attr;
> +}

This function isn't a predicate, but see the next block...

> +/* Function to handle this attribute.  NODE points to the node to which
> +   the attribute is to be applied.  If a DECL, it should be modified in
> +   place; if a TYPE, a copy should be created.  NAME is the name of the
> +   attribute (possibly with leading or trailing __).  ARGS is the TREE_LIST
> +   of the arguments (which may be NULL).  FLAGS gives further information
> +   about the context of the attribute.  Afterwards, the attributes will
> +   be added to the DECL_ATTRIBUTES or TYPE_ATTRIBUTES, as appropriate,
> +   unless *NO_ADD_ATTRS is set to true (which should be done on error,
> +   as well as in any other cases when the attributes should not be added
> +   to the DECL or TYPE).  Depending on FLAGS, any attributes to be
> +   applied to another type or DECL later may be returned;
> +   otherwise the return value should be NULL_TREE.  This pointer may be
> +   NULL if no special handling is required beyond the checks implied
> +   by the rest of this structure.  */
> +
> +static tree
> +mips_interrupt_attribute (tree *node ATTRIBUTE_UNUSED,
> +			  tree name ATTRIBUTE_UNUSED, tree args,
> +			  int flags ATTRIBUTE_UNUSED, bool *no_add_attrs)
> +{
> +  /* We want to validate that the argument isn't bogus. There should
> +     be one and only one argument in the args TREE_LIST and it should
> +     be an identifier of the form "single" or "ipl[0-7]".  */
> +
> +  /* We can assert one argument since that should be enforced by
> +     the parser from the attribute table.  */
> +  gcc_assert (TREE_CHAIN (args) == NULL);
> +
> +  if (TREE_CODE (TREE_VALUE (args)) != IDENTIFIER_NODE
> +      || (strcmp ("single", IDENTIFIER_POINTER (TREE_VALUE (args))) != 0
> +          && ((strncmp ("ipl", IDENTIFIER_POINTER (TREE_VALUE (args)), 3) != 0
> +               && strncmp ("IPL", IDENTIFIER_POINTER (TREE_VALUE(args)),3)!= 0)
> +	      || IDENTIFIER_POINTER (TREE_VALUE (args))[3] > '7'
> +	      || IDENTIFIER_POINTER (TREE_VALUE (args))[3] < '0')))

This allows things like "ipl3foobar", unless I'm missing something.
IMO, it'd be more elegant to split out the IDENTIFIER_POINTER checks
into a new function.  E.g.:

static int
mips_parse_interrupt_priority (const char *type)
{
  if (strcmp (type, "single") == 0)
    return 0;

  if ((strncmp (type, "ipl", 3) == 3 || strncmp (type, "IPL", 3) == 3)
      && IN_RANGE (type[3], '0', '7')
      && type[4] == 0)
    return type[3] - '0';

  return -1;
}

(Partial case-insensitivity doesn't seem very C-like, but I realise
we probably have to keep this for backwards compatibility.)

We could then rename mips_interrupt_type_p to mips_interrupt_priority
and return the same kind of int.  That's what non-boolean callers want,
rather than the raw tree, and this isn't speed-critical code.

> +    {
> +      error ("Interrupt priority must be specified as 'single' or 'ipl0' - 'ipl7'");

s/'foo'/%<foo%>/.  Same for other error messages later in the file.

> +/* Function to handle at_vector attribute.  */
> +
> +static tree
> +mips_at_vector_attribute (tree *node ATTRIBUTE_UNUSED,
> +			  tree name ATTRIBUTE_UNUSED, tree args,
> +			  int flags ATTRIBUTE_UNUSED, bool *no_add_attrs)
> +{
> +  tree decl = *node;
> +  char scn_name[11];
> +
> +  /* If this attribute isn't on the actual function declaration, we
> +     ignore it.  */
> +  if (TREE_CODE (decl) != FUNCTION_DECL)
> +    return NULL_TREE;

I'm not sure offhand why this is the right thing to do.  A more
detailed comment would be nice.

> +  /* The argument must be an integer constant between 0 and 63.  */
> +  if (TREE_CODE (TREE_VALUE (args)) != INTEGER_CST ||
> +      (int)TREE_INT_CST_LOW (TREE_VALUE (args)) < 0 ||
> +      (int)TREE_INT_CST_LOW (TREE_VALUE (args)) > 63)
> +    {
> +      *no_add_attrs = 1;
> +      error ("IRQ number must be an integer between 0 and 63");
> +      return NULL_TREE;
> +    }

I think this should be:

  if (!host_integer_p (TREE_VALUE (args))
      || !IN_RANGE (TREE_INT_CST_LOW (TREE_VALUE (args)), 0, 63))

> +
> +  /* Now mark the decl as going into the section for the indicated vector.  */
> +  sprintf (scn_name, ".vector_%d", (int)TREE_INT_CST_LOW (TREE_VALUE (args)));

Space after casts.

> +/* Utility function to add an entry to the vector dispatch list.  */

s/to the/to the head of/

Add something like:

  TARGET_NAME and VECTOR_NUMBER are field values for the new list element.

> +  /* The vector attribute has a comma delimited list of IRQ #'s as
> +     arguments. At least one must be present.  */

s/#'s/numbers/
Two spaces after "."

> +  gcc_assert (args);
> +  while (args)
> +    {
> +      /* The argument must be an integer constant between 0 and 63.  */
> +      if (TREE_CODE (TREE_VALUE (args)) != INTEGER_CST
> +	  || (int) TREE_INT_CST_LOW (TREE_VALUE (args)) < 0
> +	  || (int) TREE_INT_CST_LOW (TREE_VALUE (args)) > 63)

As above.  Let's split this out into a separate function:

static bool
mips_validate_interrupt_number_p (tree arg)
{
  if (!host_integerp (arg) || !IN_RANGE (TREE_INT_CST_LOW (arg), 0, 63))
    {
      error ("IRQ number must be an integer between 0 and 63");
      return false;
    }
  return true;
}

and use it for at_vector too.

> +/* Implement TARGET_ASM_FILE_END.  */
> +
> +static void
> +mips_file_end (void)
> +{
> +  struct vector_dispatch_spec *dispatch_entry;
> +
> +  fputs ("\n# MIPS vector dispatch table\n", asm_out_file);

Let's not write this if the table is empty.  I suggest renaming
the function to mips_write_vector_dispatch_list and making it
a separate subroutine of mips_file_end, guarded by:

  if (vector_dispatch_list)
    mips_write_vector_dispatch_list ();

Then "entry" would be fine as a local variable name, rather than
"dispatch_entry".

> +  /* Output the vector dispatch functions specified in this translation
> +     unit, if any.  */
> +  for (dispatch_entry = vector_dispatch_list_head; dispatch_entry;
> +       dispatch_entry = dispatch_entry->next)
> +    {
> +      fprintf (asm_out_file, "\t.section\t.vector_%d,\"ax\",@progbits\n",
> +	       dispatch_entry->vector_number);
> +      fprintf (asm_out_file, "\t.align\t2\n");
> +      fprintf (asm_out_file, "\t.set\tnomips16\n");
> +      fprintf (asm_out_file, "\t.ent\t__vector_dispatch_%d\n",
> +	       dispatch_entry->vector_number);
> +      fprintf (asm_out_file, "__vector_dispatch_%d:\n",
> +	       dispatch_entry->vector_number);
> +      fprintf (asm_out_file, "\tj\t%s\n", dispatch_entry->target);
> +      fprintf (asm_out_file, "\t.end\t__vector_dispatch_%d\n",
> +	       dispatch_entry->vector_number);
> +      fprintf (asm_out_file, "\t.size\t__vector_dispatch_%d, .-"
> +	       "__vector_dispatch_%d\n",
> +	       dispatch_entry->vector_number,
> +	       dispatch_entry->vector_number);

Please use mips_start_function_definition and mips_end_function_definition.

> +/* Returns true if regno is a register ordinarilly not callee saved which
> +   must nevertheless be preserved by an interrupt handler function.  */
> +
> +static bool
> +mips_register_interrupt_context_p (unsigned int regno)
> +{
> +  /* return true for v0, v1, a0-a3, t0-t9 and ra  */
> +  if ((regno >= 2 && regno <= 15)	/* v0, v1, a0-a3, t0-t7 */
> +      || regno == 24			/* t8 */
> +      || regno == 25			/* t9 */
> +      || regno == 31)			/* ra */
> +    return true;
> +
> +  return false;
> +}

Why not $1 as well?  I think it would be easier to have:

#define KERNEL_REG_P(REGNO) \
  IN_RANGE (REGNO, GP_REG_FIRST + 26, GP_REG_FIRST + 27)

in the GP_REG_P block of macros.  Then the condition for
saving would be:

  regno != GPR_FIRST
  && !KERNEL_REG_P (regno)
  && regno != STACK_POINTER_REGNUM)

I think we should require -msoft-float for interrupt functions,
otherwise we'd get silent miscompilation if FPU operations
crept in.

Combined with this...

> +  if (current_function_type == SOFTWARE_CONTEXT_SAVE)
> +    {
> +      /* If this is a leaf function, we can use our analysis of the code
> +	 to determine which registers are necessary to save and restore.
> +	 If it's not a leaf function, we have to make conservative
> +	 assumptions that the called function(s) will tromp all over all of
> +	 the call used registers, so we need to save those regardless.  */
> +      if (current_function_is_leaf)
> +        {
> +	  return (crtl->saves_all_registers || df_regs_ever_live_p (regno))
> +		 && (!call_really_used_regs[regno]
> +		     || mips_register_interrupt_context_p (regno));
> +	}
> +      else
> +        {
> +	  /* For non-leaf functions, we save everything we do for normal
> +	     functions plus all of the interrupt context.  */
> +	  return ((crtl->saves_all_registers || df_regs_ever_live_p (regno))
> +		  && !call_really_used_regs[regno])
> +		 || mips_register_interrupt_context_p (regno);
> +        }
> +    }
> +  else if (current_function_type == SRS_CONTEXT_SAVE)
> +    {
> +      /* If we're using a shadow set, we don't need to save any GPR */
> +      if (GP_REG_P (regno))
> +	return false;
> +    }
> +

...I think the control flow is getting too complicated.  Let's make
mips_save_reg_p simply do:

  return (mips_cfun_may_clobber_reg_p (regno)
          && mips_cfun_call_saved_reg_p (regno));

It should then be easier to fit the new logic into these two
subroutines.

> +  /* Get the current function type.  */
> +  if (current_function_type == DONT_KNOW)
> +    {
> +      tree ipl_tree;
> +      if ((ipl_tree = mips_interrupt_type_p (TREE_TYPE (current_function_decl)))
> +	  != NULL)

Move the assignment outside the "if".  (Much easier to read than
a split line.)

> +      /* If this interrupt is using a shadow register set, we need to
> +         get the stack pointer from the previous register set.  */
> +      /* Trumping that concern, at least for the time being, is that we
> +         want the first four instructions of the interrupt handler to be
> +         the same for all handler functions.  This lets there be cache lines
> +         locked to those instructions, lowering the latency.  */
> +      /* if (current_function_type == SRS_CONTEXT_SAVE) */

I don't understand the reason for commenting this line out.  The SRS and
non-SRS prologues diverge within the first four instructions anyway.
Also, unless my counting has gone awry:

> +	  fprintf (file, "\trdpgpr\t$sp, $sp\n");
> +
> +      /* Start at the uppermost location for saving.  */
> +      offset = cfun->machine->frame.int_sp_offset;
> +
> +      /* We want the first four instructions at the top of the interrupt
> +	 to be common across all software save handlers so an application
> +	 can lock the cache lines to those instructions and reduce the
> +	 latency.  */
> +      if (current_function_type == SOFTWARE_CONTEXT_SAVE)
> +        {
> +          /* This is for non-SRS interrupts.  We need to differentiate and
> +             generate different code for highest priority handlers.
> +             We don't need to move the Cause IPL into the SR for the
> +             highest priority interrupt since we're not allowing nested
> +             interrupts at that level.  All interrupts are disabled in that
> +             case (see below loading of SR), so the IPL value in SR is not
> +             relevent.  */
> +
> +          /* Load the Cause RIPL into k0.  */
> +          fprintf (file, "\tmfc0\t$k0, $13\n");
> +
> +          /* Load EPC into k1.  */
> +          fprintf (file, "\tmfc0\t$k1, $14\n");
> +        }
> +
> +      /* Allocate the stack space to save the registers.  If more space
> +         needs to be allocated, the expand_prologue() function handled
> +         it.  */
> +      fprintf (file, "\taddiu\t$sp, $sp, " HOST_WIDE_INT_PRINT_DEC "\n",
> +	       -step1);

...this stack adjustment is one of the first four instructions,
and -step1 is function-specific.  How much commonality do we get
in practice?

I'm not happy about doing this in output_function_prologue.
Emitting blockages after each instruction isn't nice either,
but it's probably better.  Same goes for the epilogue,
except that there should be no need for blockages there.

> @@ -9433,7 +10028,19 @@ mips_expand_epilogue (bool sibcall_p)
>        else
>  	regno = GP_REG_FIRST + 31;
>        mips_expand_before_return ();
> -      emit_jump_insn (gen_return_internal (gen_rtx_REG (Pmode, regno)));
> +
> +      /* non-interrupt functions generate a return instruction here.  */
> +      if (current_function_type == NON_INTERRUPT)
> +	emit_jump_insn (gen_return_internal (gen_rtx_REG (Pmode, regno)));
> +      else
> +	{
> +	  /* Interrupt functions need to emit a placeholder instruction
> +	     so that this pattern will return a non-empty expansion.
> +	     We don't want anything below this (there shouldn't be any
> +	     RTL following this, anyway) moved above it, so we'll use a
> +	     blockage. */
> +	  emit_insn (gen_blockage ());
> +	}

With the suggested epilogue changes, you should have a (return) pattern
for interrupt functions that expands to "eret".

> +/* Implement EPILOGUE_USES.  */
> +
> +bool
> +mips_epilogue_uses (unsigned int regno)
> +{
> +  /* Say that the epilogue uses the return address register.  Note that
> +     in the case of sibcalls, the values "used by the epilogue" are
> +     considered live at the start of the called function.
> +
> +     If using a GOT, say that the epilogue also uses GOT_VERSION_REGNUM.
> +     See the comment above load_call<mode> for details.  */
> +  if (regno == 31 || (TARGET_USE_GOT && (regno) == GOT_VERSION_REGNUM))
> +    return true;

This would be better as two separate statements (with separate comments).

> +/* A few bitfield locations for the coprocessor registers.  */
> +#define CAUSE_IPL	10
> +#define SR_IPL		10
> +#define SR_IE		0
> +#define SR_EXL		1
> +#define SR_ERL		2

SR_ERL isn't used.  We might as well leave it out until we need it.
I'd prefer seperate comments for the cause and status registers.

The docs need a bit of type-editing, but it's a bit late in the
day for me to do that. ;)

Since you're using a lot of MIPS*r2 features, you should check
for a revision 2 ISA.

Sorry that the message seems so negative.  I like the idea,
and it'd be nice to get the patch tweaked and committed.

Richard