[PATCH] MIPS function attributes for interrupt handlers

[PATCH] MIPS function attributes for interrupt handlers

[Fu, Chao-Ying]

Hi All,

  Four new function attributes are added for MIPS interrupt handlers.
They are "naked", "interrupt", "vector", and "at_vector".  This patch
is ported from Microchip GCC source code for PIC32 that can be downloaded
from the following link (in the middle of the web page).
http://www.microchip.com/stellent/idcplg?IdcService=SS_GET_PAGE&nodeId=2615&dDocName=en532454
NOTE: The equivalent pragma support is not ported.

  For the usage of these attributes, please check the attached patch
for "extend.texi".  Reading Chapter 3 "Interrupts" of "MPLAB C32 C Compiler User's Guide"  
http://ww1.microchip.com/downloads/en/DeviceDoc/MPLAB%20C32%20User%20Guide%2051686a.pdf
helps a lot.

  Some parts of interrupt handler supports are specific to PIC32, ex:
the shadow register set is tied to the interrupt priority level 7,
and the device is a MIPS32r2 CPU.
But, these can be changed to be more general in the future, if other
MIPS devices come out.

  Is this patch ok?  Thanks a lot!

Regards,
Chao-ying

gcc/ChangeLog
2008-10-14  Chao-ying Fu  <fu@mips.com>
		James Grosbach <james.grosbach@microchip.com>

	* config/mips/mips.c (mips_frame_info): Add int_save_offset, int_sp_offset,
	has_interrupt_context_p, has_hilo_context_p for interupt info.
	(mips_attribute_table): Add interrupt, vector, at_vector, naked.
	(function_type_tag): New enum.
	(current_function_type): New global variable.
	(vector_dispatch_spec): New structure.
	(mips_naked_decl_p, mips_interrupt_type_p, mips_interrupt_attribute,
	mips_at_vector_attribute, mips_add_vector_dispatch_entry,
	mips_vector_attribute): New functions.
	(mips_function_ok_for_sibcall): Return false for interrupt handlers.
	(mips_file_end): New function.
	(mips_register_interrupt_context_p): New function.
	(mips_save_reg_p): Check registers for interrupt handlers.
	(mips_compute_frame_info): Add supports for interrupt context.
	(mips_output_function_prologue, mips_output_function_epilogue):
	Output code for interrupt handlers.
	(mips_expand_prologue): Just return for naked functions.
	Support interrupt handlers.
	(mips_expand_epilogue): Support naked functions.
	Support interrupt handlers.
	(mips_can_use_return_insn): Return false for interrupt handlers
	and naked functions.
	(mips_epilogue_uses): New function.
	(TARGET_ASM_FILE_END): Define.

	* config/mips/mips.md (return_from_epilogue): New instruction
	that will be called for interrupt handlers.
	
	* config/mips/mips-protos.h (mips_epilogue_uses): Declare.

	* config/mips/mips.h (CAUSE_IPL, SR_IPL, SR_IE, SR_EXL, SR_ERL):
	New defines.
	(EPILOGUE_USES): Change to a function call.
	
	* doc/extend.texi (Function Attributes): Document interrupt,
	at_vector, vector, naked for MIPS.

gcc/testsuite/ChangeLog
2008-10-14  Chao-ying Fu  <fu@mips.com>

        * gcc.target/mips/interrupt_handler.c: New tests.
	
Index: gcc4x/gcc/gcc/config/mips/mips.c
===================================================================
--- gcc4x.orig/gcc/gcc/config/mips/mips.c	2008-10-13 17:29:33.000000000 -0700
+++ gcc4x/gcc/gcc/config/mips/mips.c	2008-10-13 18:23:06.000000000 -0700
@@ -265,20 +265,32 @@ struct mips_frame_info GTY(()) {
   unsigned int num_gp;
   unsigned int num_fp;
 
-  /* The offset of the topmost GPR and FPR save slots from the top of
-     the frame, or zero if no such slots are needed.  */
+  /* The number of interrupt reg saved.  */
+  unsigned int num_interrupt_reg;
+
+  /* The offset of the topmost GPR and FPR and interrupt regs save slots from
+     the top of the frame, or zero if no such slots are needed.  */
   HOST_WIDE_INT gp_save_offset;
   HOST_WIDE_INT fp_save_offset;
+  HOST_WIDE_INT int_save_offset;
 
   /* Likewise, but giving offsets from the bottom of the frame.  */
   HOST_WIDE_INT gp_sp_offset;
   HOST_WIDE_INT fp_sp_offset;
+  HOST_WIDE_INT int_sp_offset;
 
   /* The offset of arg_pointer_rtx from frame_pointer_rtx.  */
   HOST_WIDE_INT arg_pointer_offset;
 
   /* The offset of hard_frame_pointer_rtx from frame_pointer_rtx.  */
   HOST_WIDE_INT hard_frame_pointer_offset;
+
+  /* True if interrupt context is saved.  */
+  bool has_interrupt_context_p;
+
+  /* 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];
 };
 
 struct machine_function GTY(()) {
@@ -542,6 +554,10 @@ const enum reg_class mips_regno_to_class
   ALL_REGS,	ALL_REGS,	ALL_REGS,	ALL_REGS
 };
 
+static tree mips_interrupt_attribute (tree *, tree, tree, int, bool *);
+static tree mips_vector_attribute (tree *, tree, tree, int, bool *);
+static tree mips_at_vector_attribute (tree *, tree, tree, int, bool *);
+
 /* The value of TARGET_ATTRIBUTE_TABLE.  */
 const struct attribute_spec mips_attribute_table[] = {
   /* { name, min_len, max_len, decl_req, type_req, fn_type_req, handler } */
@@ -554,8 +570,34 @@ const struct attribute_spec mips_attribu
      code generation but don't carry other semantics.  */
   { "mips16", 	   0, 0, true,  false, false, NULL },
   { "nomips16",    0, 0, true,  false, false, NULL },
+  /* Allow functions to be specified as interrupt handlers */
+  { "interrupt",   1, 1, false, true,  true, mips_interrupt_attribute },
+  { "vector",      1, 64,true,  false, false, mips_vector_attribute },
+  { "at_vector",   1, 1, true,  false, false, mips_at_vector_attribute },
+  { "naked",       0, 0, true,  false, false, NULL },
   { NULL,	   0, 0, false, false, false, NULL }
 };
+
+/* 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;
+
+/* 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.  */
+struct vector_dispatch_spec
+{
+  const char *target;	/* Target function name.  */
+  int vector_number;	/* Exception vector table number.  */
+  struct vector_dispatch_spec *next;
+};
+struct vector_dispatch_spec *vector_dispatch_list_head;
 \f
 /* A table describing all the processors GCC knows about.  Names are
    matched in the order listed.  The first mention of an ISA level is
@@ -1172,6 +1214,162 @@ mips_nomips16_decl_p (const_tree decl)
   return lookup_attribute ("nomips16", DECL_ATTRIBUTES (decl)) != NULL;
 }
 
+/* Predicate to test for "naked" function attribute.  */
+
+static bool
+mips_naked_decl_p (const_tree decl)
+{
+  return lookup_attribute ("naked", DECL_ATTRIBUTES (decl)) != NULL;
+}
+
+/* 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;
+}
+
+/* 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')))
+    {
+      error ("Interrupt priority must be specified as 'single' or 'ipl0' - 'ipl7'");
+      *no_add_attrs = 1;
+      return NULL_TREE;
+    }
+
+  return NULL_TREE;
+}
+
+/* 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;
+
+  if (DECL_SECTION_NAME (decl) != NULL_TREE)
+    {
+      error ("the 'at_vector' attribute cannot be used with the 'section' attribute");
+      *no_add_attrs = true;
+      return NULL_TREE;
+    }
+
+  /* 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;
+    }
+
+  /* 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)));
+  DECL_SECTION_NAME (decl) = build_string (strlen (scn_name), scn_name);
+
+  return NULL_TREE;
+}
+
+/* Utility function to add an entry to the vector dispatch list.  */
+
+static void
+mips_add_vector_dispatch_entry (const char *target_name, int vector_number)
+{
+  struct vector_dispatch_spec *dispatch_entry;
+
+  /* Add the vector to the list of dispatch functions to emit.  */
+  dispatch_entry = ggc_alloc (sizeof (struct vector_dispatch_spec));
+  dispatch_entry->next = vector_dispatch_list_head;
+  dispatch_entry->target = target_name;
+  dispatch_entry->vector_number = vector_number;
+  vector_dispatch_list_head = dispatch_entry;
+}
+
+/* Function to handle vector attribute.  */
+
+static tree
+mips_vector_attribute (tree *node ATTRIBUTE_UNUSED,
+		       tree name ATTRIBUTE_UNUSED, tree args,
+		       int flags ATTRIBUTE_UNUSED, bool *no_add_attrs)
+{
+  tree decl = *node;
+
+  /* If this attribute isn't on the actual function declaration, we
+     ignore it.  */
+  if (TREE_CODE (decl) != FUNCTION_DECL)
+    return NULL_TREE;
+
+  /* The vector attribute has a comma delimited list of IRQ #'s as
+     arguments. At least one must be present.  */
+  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)
+	{
+	  *no_add_attrs = 1;
+	  error ("IRQ number must be an integer between 0 and 63");
+	  return NULL_TREE;
+	}
+
+      /* Add the vector to the list of dispatch functions to emit.  */
+      mips_add_vector_dispatch_entry (IDENTIFIER_POINTER (DECL_NAME (*node)),
+				      (int) TREE_INT_CST_LOW (TREE_VALUE (args)));
+
+      args = TREE_CHAIN (args);
+    }
+
+  return NULL_TREE;
+}
+
 /* Return true if function DECL is a MIPS16 function.  Return the ambient
    setting if DECL is null.  */
 
@@ -6186,6 +6384,11 @@ mips_function_ok_for_sibcall (tree decl,
   if (!TARGET_SIBCALLS)
     return false;
 
+  /* Cannot handle sibcalls from interrupt handler functions since
+     we need extra epilogue code.  */
+  if (mips_interrupt_type_p (TREE_TYPE (current_function_decl)))
+    return false;
+
   /* We can't do a sibcall if the called function is a MIPS16 function
      because there is no direct "jx" instruction equivalent to "jalx" to
      switch the ISA mode.  We only care about cases where the sibling
@@ -7829,6 +8032,38 @@ mips_file_start (void)
 	     ASM_COMMENT_START,
 	     mips_small_data_threshold, mips_arch_info->name, mips_isa);
 }
+
+/* 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);
+
+  /* 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);
+    }
+}
 \f
 /* Make the last instruction frame-related and note that it performs
    the operation described by FRAME_PATTERN.  */
@@ -8426,6 +8661,22 @@ mips_global_pointer (void)
   return GLOBAL_POINTER_REGNUM;
 }
 
+/* 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;
+}
+
 /* Return true if the current function must save register REGNO.  */
 
 static bool
@@ -8439,6 +8690,35 @@ mips_save_reg_p (unsigned int regno)
     return true;
 
   /* Check call-saved registers.  */
+  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;
+    }
+
   if ((crtl->saves_all_registers || df_regs_ever_live_p (regno))
       && !call_really_used_regs[regno])
     return true;
@@ -8498,6 +8778,10 @@ mips_save_reg_p (unsigned int regno)
       C |  callee-allocated save area   |
 	|  for register varargs         |
 	|                               |
+	+-------------------------------+ <-- frame_pointer_rtx + int_sp_offset
+	|                               |       + UNITS_PER_WORD
+	|  interrupt save area          |
+	|                               |
 	+-------------------------------+ <-- frame_pointer_rtx + fp_sp_offset
 	|                               |       + UNITS_PER_HWFPVALUE
 	|  FPR save area                |
@@ -8541,12 +8825,53 @@ mips_compute_frame_info (void)
   HOST_WIDE_INT offset, size;
   unsigned int regno, i;
 
+  /* 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)
+	{
+	  unsigned int interrupt_priority;
+	  /* The priority can be either "single" or "ipl[0..7]"
+	     When the interrupt handler is for single interrupt mode, we
+	     treat it as a software context save handler. */
+	  if (strcmp (IDENTIFIER_POINTER (TREE_VALUE (ipl_tree)), "single")
+	      == 0)
+	    interrupt_priority = 0;
+	  else
+	    interrupt_priority = IDENTIFIER_POINTER (TREE_VALUE (ipl_tree))[3]
+						 - '0';
+	  if (interrupt_priority == 7)
+	    current_function_type = SRS_CONTEXT_SAVE;
+	  else
+	    current_function_type = SOFTWARE_CONTEXT_SAVE;
+
+	  if (mips_naked_decl_p (current_function_decl))
+	    error ("interrupt handler functions cannot also be naked functions");
+	}
+      else
+	current_function_type = NON_INTERRUPT;
+    }
+
   frame = &cfun->machine->frame;
   memset (frame, 0, sizeof (*frame));
   size = get_frame_size ();
 
   cfun->machine->global_pointer = mips_global_pointer ();
 
+  /* has_interrupt_context_p tells us whether we're saving any interrupt
+     specific data.  */
+  frame->has_interrupt_context_p =
+    (current_function_type == SOFTWARE_CONTEXT_SAVE
+     || current_function_type == SRS_CONTEXT_SAVE);
+
+  /* has_hilo_context_p is true if we need to push/pop HI and LO.  */
+  frame->has_hilo_context_p[0] = false;
+  frame->has_hilo_context_p[1] = false;
+  frame->has_hilo_context_p[2] = false;
+  frame->has_hilo_context_p[3] = false;
+
   /* The first STARTING_FRAME_OFFSET bytes contain the outgoing argument
      area and the $gp save slot.  This area isn't needed in leaf functions,
      but if the target-independent frame size is nonzero, we're committed
@@ -8626,6 +8951,62 @@ mips_compute_frame_info (void)
       frame->fp_sp_offset = offset - UNITS_PER_HWFPVALUE;
     }
 
+  /* Add in space for the interrupt context information.  */
+  if (frame->has_interrupt_context_p)
+    {
+      /* All interrupt context functions need space
+         to preserve STATUS.  */
+      frame->num_interrupt_reg++;
+
+      /* All software interrupt context functions need space
+         to preserve EPC also.  */
+      if (current_function_type == SOFTWARE_CONTEXT_SAVE)
+	frame->num_interrupt_reg++;
+
+      /* 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;
+	      }
+	    }
+	}
+    }
+
+  /* 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;
+    }
+
   /* Move above the callee-allocated varargs save area.  */
   offset += MIPS_STACK_ALIGN (cfun->machine->varargs_size);
   frame->arg_pointer_offset = offset;
@@ -8639,6 +9020,8 @@ mips_compute_frame_info (void)
     frame->gp_save_offset = frame->gp_sp_offset - offset;
   if (frame->fp_sp_offset > 0)
     frame->fp_save_offset = frame->fp_sp_offset - offset;
+  if (frame->num_interrupt_reg > 0)
+    frame->int_save_offset = frame->int_sp_offset - offset;
 
   /* MIPS16 code offsets the frame pointer by the size of the outgoing
      arguments.  This tends to increase the chances of using unextended
@@ -8977,6 +9360,127 @@ mips_output_function_prologue (FILE *fil
      pointer.  This is needed for thunks, since they can use either
      explicit relocs or assembler macros.  */
   mips_output_cplocal ();
+
+  /* If this is an interrupt function, we need to save that
+     context first.  We don't want to use the generic
+     mips_for_each_saved_reg() function because we want to
+     leave defined values in K0 and K1.  */
+  if (cfun->machine->frame.has_interrupt_context_p)
+    {
+      unsigned int i;
+      HOST_WIDE_INT step1;
+      HOST_WIDE_INT offset;
+      HOST_WIDE_INT tsize = cfun->machine->frame.total_size;
+
+      step1 = MIN (tsize, MIPS_MAX_FIRST_STACK_STEP);
+
+      /* 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) */
+	  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);
+
+      if (current_function_type == SOFTWARE_CONTEXT_SAVE)
+	{
+	  /* Push EPC into its stack slot.  */
+	  fprintf (file, "\tsw\t$k1, " HOST_WIDE_INT_PRINT_DEC "($sp)\n",
+		   offset);
+	  offset -= GET_MODE_SIZE (SImode);
+	}
+
+      /* Push status into its stack slot.  */
+      fprintf (file, "\tmfc0\t$k1, $12\n");
+      if (current_function_type == SOFTWARE_CONTEXT_SAVE)
+	{
+	  /* Right justify the RIPL in k0,
+	     and avoid Read after Write interlock on k1.  */
+          fprintf (file, "\tsrl\t$k0, $k0, %d\n", CAUSE_IPL);
+	}
+
+      fprintf (file, "\tsw\t$k1, " HOST_WIDE_INT_PRINT_DEC "($sp)\n", offset);
+      offset -= GET_MODE_SIZE (SImode);
+
+      if (current_function_type == SOFTWARE_CONTEXT_SAVE)
+	{
+	  /* Insert the RIPL into our copy of SR (k1) as the new IPL.  */
+	  fprintf (file, "\tins\t$k1, $k0, %d, 6\n", SR_IPL);
+	}
+
+      /* We may need to also preserve HI/LO. We want to do this as late
+         as possible in the prologue sequence so that if we interrupted
+         during a mul/div operation, we give it as much time to complete
+         as we can before accessing HI/LO and potentially stalling the
+         pipeline.  */
+      for (i = 0; i < 4; i++)
+        if (cfun->machine->frame.has_hilo_context_p[i])
+	  {
+	    if (i == 0)
+	      fprintf (file, "\tmflo\t$k0\n");
+	    else
+	      fprintf (file, "\tmflo\t$k0, $ac%d\n", i);
+	    fprintf (file, "\tsw\t$k0, " HOST_WIDE_INT_PRINT_DEC "($sp)\n",
+		     offset);
+	    offset -= GET_MODE_SIZE (SImode);
+	    if (i == 0)
+	      fprintf (file, "\tmfhi\t$k0\n");
+	    else
+	      fprintf (file, "\tmfhi\t$k0, $ac%d\n", i);
+	    fprintf (file, "\tsw\t$k0, " HOST_WIDE_INT_PRINT_DEC "($sp)\n",
+		     offset);
+	    offset -= GET_MODE_SIZE (SImode);
+	}
+
+      /* Enable interrupts and clear the UM, ERL and EXL bits.
+	 IE is already the correct value, so we don't have to do
+	 anything explicit there for software context save. For SRS,
+	 we're in the highest priority alreadys, so we don't want
+	 to reenable interrupts at all, and should thus also clear IE.  */
+      if (current_function_type == SOFTWARE_CONTEXT_SAVE)
+	fprintf (file, "\tins\t$k1, $0, %d, %d\n", SR_EXL, 4);
+      else
+	{
+	  fprintf (file, "\tins\t$k1, $0, %d, %d\n", SR_IE, 5);
+
+	  /* In current IPL7 is the ONLY one with a SRS so we can set the
+	     IPL in Status directly IPL=bits 15-10.  */
+	  fprintf (file, "\tori\t$k1, $k1, 0x1c00\n");
+	}
+
+      fprintf (file, "\tmtc0\t$k1, $12\n");
+    }
 }
 
 /* Implement TARGET_OUTPUT_FUNCTION_EPILOGUE.  */
@@ -8987,6 +9491,72 @@ mips_output_function_epilogue (FILE *fil
 {
   const char *fnname;
 
+  /* If this is an interrupt handler function, we need to process
+     the additional interrupt context as well.  */
+  if (cfun->machine->frame.has_interrupt_context_p)
+    {
+      unsigned int i;
+      HOST_WIDE_INT offset;
+      HOST_WIDE_INT tsize = cfun->machine->frame.total_size;
+      HOST_WIDE_INT step1 = MIN (tsize, MIPS_MAX_FIRST_STACK_STEP);
+
+      /* Disable interrupts.  */
+      if (current_function_type == SOFTWARE_CONTEXT_SAVE)
+	{
+	  fprintf (file, "\tdi\n");
+	  fprintf (file, "\tehb\n");
+	}
+
+      /* Restore the original HI/LO if required.  */
+      for (i = 0; i < 4; i++)
+	if (cfun->machine->frame.has_hilo_context_p[i])
+	  {
+	    offset = cfun->machine->frame.int_sp_offset
+		     - GET_MODE_SIZE (SImode);
+
+            if (current_function_type == SOFTWARE_CONTEXT_SAVE)
+	      offset -= GET_MODE_SIZE (SImode);
+
+	    fprintf (file, "\tlw\t$k0, " HOST_WIDE_INT_PRINT_DEC "($sp)\n",
+		     offset);
+	    if (i == 0)
+	      fprintf (file, "\tmtlo\t$k0\n");
+	    else
+	      fprintf (file, "\tmtlo\t$k0, $ac%d\n", i);
+	    offset -= GET_MODE_SIZE (SImode);
+	    fprintf (file, "\tlw\t$k0, " HOST_WIDE_INT_PRINT_DEC "($sp)\n",
+		     offset);
+	    if (i == 0)
+	      fprintf (file, "\tmthi\t$k0\n");
+	    else
+	      fprintf (file, "\tmthi\t$k0, $ac%d\n", i);
+	  }
+
+      offset = cfun->machine->frame.int_sp_offset;
+      if (current_function_type == SOFTWARE_CONTEXT_SAVE)
+	{
+	  /* Restore the original EPC.  */
+          fprintf (file, "\tlw\t$k0, " HOST_WIDE_INT_PRINT_DEC "($sp)\n",
+		   offset);
+	  fprintf (file, "\tmtc0\t$k0, $14\n");
+	  offset -= GET_MODE_SIZE (SImode);
+	}
+
+      /* Get the original Status into K0.  */
+      fprintf (file, "\tlw\t$k0, " HOST_WIDE_INT_PRINT_DEC "($sp)\n", offset);
+
+      /* Deallocate the stack space.  */
+      if (current_function_type == SOFTWARE_CONTEXT_SAVE)
+	fprintf (file, "\taddiu\t$sp, $sp, " HOST_WIDE_INT_PRINT_DEC "\n",
+		 step1);
+
+      /* Restore the original status.  */
+      fprintf (file, "\tmtc0\t$k0, $12\n");
+
+      /* Return. */
+      fprintf (file, "\teret\n");
+    }
+
   /* Reinstate the normal $gp.  */
   SET_REGNO (pic_offset_table_rtx, GLOBAL_POINTER_REGNUM);
   mips_output_cplocal ();
@@ -9004,6 +9574,9 @@ mips_output_function_epilogue (FILE *fil
      exactly matches the name used in ASM_DECLARE_FUNCTION_NAME.  */
   fnname = XSTR (XEXP (DECL_RTL (current_function_decl), 0), 0);
   mips_end_function_definition (fnname);
+
+  /* Reset current_function_type.  */
+  current_function_type = DONT_KNOW;
 }
 \f
 /* Save register REG to MEM.  Make the instruction frame-related.  */
@@ -9119,13 +9692,19 @@ mips_expand_prologue (void)
   if (cfun->machine->global_pointer > 0)
     SET_REGNO (pic_offset_table_rtx, cfun->machine->global_pointer);
 
+  /* If this function is specified as a naked function, just return without
+     expanding any code.  */
+  if (mips_naked_decl_p (current_function_decl))
+    return;
+
   frame = &cfun->machine->frame;
   size = frame->total_size;
 
   /* Save the registers.  Allocate up to MIPS_MAX_FIRST_STACK_STEP
      bytes beforehand; this is enough to cover the register save area
      without going out of range.  */
-  if ((frame->mask | frame->fmask) != 0)
+  if (((frame->mask | frame->fmask) != 0)
+      || cfun->machine->frame.has_interrupt_context_p)
     {
       HOST_WIDE_INT step1;
 
@@ -9156,10 +9735,16 @@ mips_expand_prologue (void)
  	}
       else
  	{
-	  insn = gen_add3_insn (stack_pointer_rtx,
-				stack_pointer_rtx,
-				GEN_INT (-step1));
-	  RTX_FRAME_RELATED_P (emit_insn (insn)) = 1;
+	  /* If this is an interrupt function, this first stack allocation
+	     will be performed by the static frame code and doesn't need
+	     to be done here.  */
+	  if (current_function_type == NON_INTERRUPT)
+	    {
+	      insn = gen_add3_insn (stack_pointer_rtx,
+				    stack_pointer_rtx,
+				    GEN_INT (-step1));
+	      RTX_FRAME_RELATED_P (emit_insn (insn)) = 1;
+	    }
 	  size -= step1;
 	  mips_for_each_saved_reg (size, mips_save_reg);
 	}
@@ -9306,6 +9891,15 @@ mips_expand_epilogue (bool sibcall_p)
       return;
     }
 
+  /* If this function is specified as a naked function, we don't want
+     any actual epilogue code.  We still need to have something for
+     the "return" statement branches to target.  */
+  if (mips_naked_decl_p (current_function_decl))
+    {
+      emit_jump_insn (gen_return_from_epilogue ());
+      return;
+    }
+
   /* In MIPS16 mode, if the return value should go into a floating-point
      register, we need to call a helper routine to copy it over.  */
   if (mips16_cfun_returns_in_fpr_p ())
@@ -9331,7 +9925,8 @@ mips_expand_epilogue (bool sibcall_p)
 
   /* If we need to restore registers, deallocate as much stack as
      possible in the second step without going out of range.  */
-  if ((frame->mask | frame->fmask) != 0)
+  if ((frame->mask | frame->fmask) != 0
+      || cfun->machine->frame.has_interrupt_context_p)
     {
       step2 = MIN (step1, MIPS_MAX_FIRST_STACK_STEP);
       step1 -= step2;
@@ -9395,8 +9990,8 @@ mips_expand_epilogue (bool sibcall_p)
       /* Restore the registers.  */
       mips_for_each_saved_reg (frame->total_size - step2, mips_restore_reg);
 
-      /* Deallocate the final bit of the frame.  */
-      if (step2 > 0)
+      /* Deallocate the final bit of the frame for non-interrupts.  */
+      if (!cfun->machine->frame.has_interrupt_context_p && step2 > 0)
 	emit_insn (gen_add3_insn (stack_pointer_rtx,
 				  stack_pointer_rtx,
 				  GEN_INT (step2)));
@@ -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 ());
+	}
     }
 }
 \f
@@ -9444,6 +10051,16 @@ mips_expand_epilogue (bool sibcall_p)
 bool
 mips_can_use_return_insn (void)
 {
+  /* Interrupt handlers need to go through the epilogue.  */
+  if (current_function_type != NON_INTERRUPT)
+    return false;
+
+  /* Naked functions don't emit a return instruction directly.
+     since the prologue/epilogue is user-generated, we can't make
+     assumptions about what optimizations might be OK.  */
+  if (mips_naked_decl_p (current_function_decl))
+    return false;
+
   if (!reload_completed)
     return false;
 
@@ -14038,6 +14655,28 @@ mips_order_regs_for_local_alloc (void)
       reg_alloc_order[24] = 0;
     }
 }
+
+/* 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;
+
+  /* If the register is part of the GPRs that's saved for interrupt context,
+     we need to mark it as used by the epilogue.  */
+  if (current_function_type == SOFTWARE_CONTEXT_SAVE)
+    return mips_register_interrupt_context_p (regno);
+
+  return false;
+}
 \f
 /* Initialize the GCC target structure.  */
 #undef TARGET_ASM_ALIGNED_HI_OP
@@ -14112,6 +14751,8 @@ mips_order_regs_for_local_alloc (void)
 
 #undef TARGET_ASM_FILE_START
 #define TARGET_ASM_FILE_START mips_file_start
+#undef TARGET_ASM_FILE_END
+#define TARGET_ASM_FILE_END mips_file_end
 #undef TARGET_ASM_FILE_START_FILE_DIRECTIVE
 #define TARGET_ASM_FILE_START_FILE_DIRECTIVE true
 
Index: gcc4x/gcc/gcc/config/mips/mips.md
===================================================================
--- gcc4x.orig/gcc/gcc/config/mips/mips.md	2008-10-13 17:29:33.000000000 -0700
+++ gcc4x/gcc/gcc/config/mips/mips.md	2008-10-13 17:33:02.000000000 -0700
@@ -5652,6 +5652,13 @@
   [(set_attr "type"	"jump")
    (set_attr "mode"	"none")])
 
+(define_insn "return_from_epilogue"
+  [(return)]
+  "reload_completed"
+  "%*j\t$31%/"
+  [(set_attr "type"	"jump")
+   (set_attr "mode"	"none")])
+
 ;; Normal return.
 
 (define_insn "return_internal"
Index: gcc4x/gcc/gcc/config/mips/mips-protos.h
===================================================================
--- gcc4x.orig/gcc/gcc/config/mips/mips-protos.h	2008-10-13 17:25:41.000000000 -0700
+++ gcc4x/gcc/gcc/config/mips/mips-protos.h	2008-10-13 17:33:02.000000000 -0700
@@ -330,4 +330,6 @@ extern void mips_expand_atomic_qihi (uni
 
 extern void mips_expand_vector_init (rtx, rtx);
 
+extern bool mips_epilogue_uses (unsigned int);
+
 #endif /* ! GCC_MIPS_PROTOS_H */
Index: gcc4x/gcc/gcc/config/mips/mips.h
===================================================================
--- gcc4x.orig/gcc/gcc/config/mips/mips.h	2008-10-13 17:29:33.000000000 -0700
+++ gcc4x/gcc/gcc/config/mips/mips.h	2008-10-13 17:33:02.000000000 -0700
@@ -1641,6 +1641,13 @@ enum mips_code_readable_setting {
 #define HI_REGNUM	(TARGET_BIG_ENDIAN ? MD_REG_FIRST : MD_REG_FIRST + 1)
 #define LO_REGNUM	(TARGET_BIG_ENDIAN ? MD_REG_FIRST + 1 : MD_REG_FIRST)
 
+/* 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
+
 /* FPSW_REGNUM is the single condition code used if !ISA_HAS_8CC.
    If ISA_HAS_8CC, it should not be used, and an arbitrary ST_REG
    should be used instead.  */
@@ -2249,9 +2256,10 @@ typedef struct mips_args {
    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.  */
-#define EPILOGUE_USES(REGNO) \
-  ((REGNO) == 31 || (TARGET_USE_GOT && (REGNO) == GOT_VERSION_REGNUM))
+   See the comment above load_call<mode> for details.
+
+   For interrupt handlers, registers in interrupt context are used.  */
+#define EPILOGUE_USES(REGNO)	(mips_epilogue_uses (REGNO))
 
 /* Treat LOC as a byte offset from the stack pointer and round it up
    to the next fully-aligned offset.  */
Index: gcc4x/gcc/gcc/doc/extend.texi
===================================================================
--- gcc4x.orig/gcc/gcc/doc/extend.texi	2008-10-13 17:25:41.000000000 -0700
+++ gcc4x/gcc/gcc/doc/extend.texi	2008-10-13 17:33:02.000000000 -0700
@@ -2363,7 +2363,7 @@ This attribute is ignored for R8C target
 
 @item interrupt
 @cindex interrupt handler functions
-Use this attribute on the ARM, AVR, CRX, M32C, M32R/D, m68k,
+Use this attribute on the ARM, AVR, CRX, M32C, M32R/D, m68k, MIPS
 and Xstormy16 ports to indicate that the specified function is an
 interrupt handler.  The compiler will generate function entry and exit
 sequences suitable for use in an interrupt handler when this attribute
@@ -2386,6 +2386,56 @@ Permissible values for this parameter ar
 On ARMv7-M the interrupt type is ignored, and the attribute means the function
 may be called with a word aligned stack pointer.
 
+Note, for the MIPS, you must specify the kind of interrupt to be handled by
+adding a parameter to the interrupt attribute like this:
+
+@smallexample
+void f () __attribute__ ((interrupt (single)));
+@end smallexample
+
+The funciton f is a single interrupt handler that deals with all interrupts.
+
+@smallexample
+void g () __attribute__ ((interrupt (ipl6)));
+@end smallexample
+
+The funciton g is an interrupt handler that handles interrupts at the
+priority level of 6.
+
+Permissible values for this parameter are: single, ipl0, ipl1, ipl2, ipl3,
+ipl4, ipl5, ipl6, ipl7.  If ipl7 is used, a shadow register set is
+used for context switch to reduce interrupt latency.  For parameters other
+than ipl7, normal load and store instructions are used for context switch.
+Note that MIPS interrupt handlers assume the stack pointer is valid at all
+time.
+
+@item at_vector
+@cindex install interrupt handler at the address of exception vector on MIPS
+Using this attribute on MIPS indicates that the specific interrupt handler
+should be located at the address of the exception vector (from 0 to 63).
+
+@smallexample
+void f ()  __attribute__ ((interrupt(single))) __attribute__((at_vector (20)));
+@end smallexample
+
+The single interrupt handler f is installed at the exception vectors 20
+which section name is ".vector_20".
+
+@item vector
+@cindex install interrupt dispatch at addresses of exception vectors on MIPS
+Using this attribute on MIPS indicates that the dispatch functions
+should be located at addresses of exception vectors (from 0 to 63)
+which jump to the real interrupt handler.  The number of vectors can be
+more than 1.
+
+@smallexample
+void g ()  __attribute__ ((interrupt(ipl6))) __attribute__((vector (3, 4)));
+@end smallexample
+
+The interrupt handler g has a priority of 6, and is the target from
+dispatch functions at exception vectors 3 (which section name is ".vector_3")
+and 4 (which section name is ".vector_4").
+
 @item interrupt_handler
 @cindex interrupt handler functions on the Blackfin, m68k, H8/300 and SH processors
 Use this attribute on the Blackfin, m68k, H8/300, H8/300H, H8S, and SH to
@@ -2524,7 +2574,7 @@ Note, This feature is currently sorried 
 
 @item naked
 @cindex function without a prologue/epilogue code
-Use this attribute on the ARM, AVR, IP2K and SPU ports to indicate that
+Use this attribute on the ARM, AVR, IP2K, MIPS and SPU ports to indicate that
 the specified function does not need prologue/epilogue sequences generated by
 the compiler.  It is up to the programmer to provide these sequences. The 
 only statements that can be safely included in naked functions are 
Index: gcc4x/gcc/gcc/testsuite/gcc.target/mips/interrupt_handler.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ gcc4x/gcc/gcc/testsuite/gcc.target/mips/interrupt_handler.c	2008-10-14 10:52:47.858502000 -0700
@@ -0,0 +1,30 @@
+/* Test attributes for interrupt handlers */
+/* { dg-do compile } */
+/* { dg-mips-options "-march=mips32r2 -O2" } */
+
+void f () { }
+void __attribute__ ((naked)) n () { }
+
+void __attribute__ ((interrupt(single))) s () { }
+void __attribute__ ((interrupt(ipl0))) v0 () { }
+void __attribute__ ((interrupt(ipl1))) v1 () { }
+void __attribute__ ((interrupt(ipl2))) v2 () { }
+void __attribute__ ((interrupt(ipl3))) v3 () { }
+void __attribute__ ((interrupt(ipl4))) v4 () { }
+void __attribute__ ((interrupt(ipl5))) v5 () { }
+void __attribute__ ((interrupt(ipl5))) v6 () { }
+void __attribute__ ((interrupt(ipl7))) v7 () { }
+
+void __attribute__ ((interrupt(single))) t () { f(); }
+void __attribute__ ((interrupt(ipl0))) u0 () { f(); }
+void __attribute__ ((interrupt(ipl1))) u1 () { f(); }
+void __attribute__ ((interrupt(ipl2))) u2 () { f(); }
+void __attribute__ ((interrupt(ipl3))) u3 () { f(); }
+void __attribute__ ((interrupt(ipl4))) u4 () { f(); }
+void __attribute__ ((interrupt(ipl5))) u5 () { f(); }
+void __attribute__ ((interrupt(ipl6))) u6 () { f(); }
+void __attribute__ ((interrupt(ipl7))) u7 () { f(); }
+
+void __attribute__ ((interrupt(single))) __attribute__ ((at_vector(0))) w () {}
+void __attribute__ ((interrupt(ipl0))) __attribute__ ((at_vector(63))) x () {}
+void __attribute__ ((interrupt(ipl7))) __attribute__ ((vector(1, 2))) y () {}

Re: [PATCH] MIPS function attributes for interrupt handlers

[Richard Sandiford]

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

Re: [PATCH] MIPS function attributes for interrupt handlers

[Adam Nemet]

"Fu, Chao-Ying" <fu@mips.com> writes:
> Index: gcc4x/gcc/gcc/config/mips/mips.c
> ===================================================================
> --- gcc4x.orig/gcc/gcc/config/mips/mips.c	2008-10-13 17:29:33.000000000 -0700
> +++ gcc4x/gcc/gcc/config/mips/mips.c	2008-10-13 18:23:06.000000000 -0700
> @@ -8977,6 +9360,127 @@ mips_output_function_prologue (FILE *fil
>       pointer.  This is needed for thunks, since they can use either
>       explicit relocs or assembler macros.  */
>    mips_output_cplocal ();
> +
> +  /* If this is an interrupt function, we need to save that
> +     context first.  We don't want to use the generic
> +     mips_for_each_saved_reg() function because we want to
> +     leave defined values in K0 and K1.  */
> +  if (cfun->machine->frame.has_interrupt_context_p)
> +    {
> +      unsigned int i;
> +      HOST_WIDE_INT step1;
> +      HOST_WIDE_INT offset;
> +      HOST_WIDE_INT tsize = cfun->machine->frame.total_size;
> +
> +      step1 = MIN (tsize, MIPS_MAX_FIRST_STACK_STEP);
> +
> +      /* 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) */
> +	  fprintf (file, "\trdpgpr\t$sp, $sp\n");

You seems to be using MIPS32r2 instructions in the prologue.  I think this
should be emitted in RTL.

> Index: gcc4x/gcc/gcc/doc/extend.texi
> ===================================================================
> --- gcc4x.orig/gcc/gcc/doc/extend.texi	2008-10-13 17:25:41.000000000 -0700
> +++ gcc4x/gcc/gcc/doc/extend.texi	2008-10-13 17:33:02.000000000 -0700
> @@ -2386,6 +2386,56 @@ Permissible values for this parameter ar
>  On ARMv7-M the interrupt type is ignored, and the attribute means the function
>  may be called with a word aligned stack pointer.
>  
> +Note, for the MIPS, you must specify the kind of interrupt to be handled by
> +adding a parameter to the interrupt attribute like this:
> +
> +@smallexample
> +void f () __attribute__ ((interrupt (single)));
> +@end smallexample
> +
> +The funciton f is a single interrupt handler that deals with all interrupts.
> +
> +@smallexample
> +void g () __attribute__ ((interrupt (ipl6)));
> +@end smallexample
> +
> +The funciton g is an interrupt handler that handles interrupts at the
> +priority level of 6.
> +
> +Permissible values for this parameter are: single, ipl0, ipl1, ipl2, ipl3,
> +ipl4, ipl5, ipl6, ipl7.  If ipl7 is used, a shadow register set is
> +used for context switch to reduce interrupt latency.  For parameters other
> +than ipl7, normal load and store instructions are used for context switch.
> +Note that MIPS interrupt handlers assume the stack pointer is valid at all
> +time.

It seems that ipl7 implies two things: use the shadow register set and that
all interrupts should remain masked.  It seems the second can be useful even
if the processor does not support shadow registers.  One idea would be to
split the two and make them explicit (e.g. use_shadow_register_set and
keep_interrupts_masked attributes).

> +@item at_vector
> +@cindex install interrupt handler at the address of exception vector on MIPS
> +Using this attribute on MIPS indicates that the specific interrupt handler
> +should be located at the address of the exception vector (from 0 to 63).
> +
> +@smallexample
> +void f ()  __attribute__ ((interrupt(single))) __attribute__((at_vector (20)));
> +@end smallexample
> +
> +The single interrupt handler f is installed at the exception vectors 20
> +which section name is ".vector_20".

I think we shouldn't suggest here that we install this function at a certain
address but be clear that all this does is to put the function into a named
section.

Adam

Re: [PATCH] MIPS function attributes for interrupt handlers

[Mark Mitchell]

Richard Sandiford wrote:

> 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.

FWIW, I tend to agree.  However, what the attribute buys you is that you
don't have to write the assembly prologue/epilogue pseudo-ops like
".globl x; .fnstart; ...", etc.  So, if that's what we really want, then
I think the semantics should be that the body of a naked function
contains *only* asm statements, and that any inputs, outputs, clobbers,
etc. contain only references to entities with static storage duration.

That would allow the compiler to spit out the bits of assembly goo which
vary from OS to OS, including things like setting the function
visibility, without getting into the messy bits where people try to
write C code inside a naked function.

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: [PATCH] MIPS function attributes for interrupt handlers

[Adam Nemet]

Mark Mitchell <mark@codesourcery.com> writes:
> FWIW, I tend to agree.  However, what the attribute buys you is that you
> don't have to write the assembly prologue/epilogue pseudo-ops like
> ".globl x; .fnstart; ...", etc.  So, if that's what we really want, then
> I think the semantics should be that the body of a naked function
> contains *only* asm statements, and that any inputs, outputs, clobbers,
> etc. contain only references to entities with static storage duration.

You can still run out of call-clobbered registers by using too many "r"s on
statics in which case it's not clear who is supposed to allocate the
call-saved registers.  I would say, no extended assembly only asm with no
arguments.

Adam

RE: [PATCH] MIPS function attributes for interrupt handlers

[Weddington, Eric]

 

> -----Original Message-----
> From: Mark Mitchell [mailto:mark@codesourcery.com] 
> Sent: Monday, October 20, 2008 3:53 PM
> To: Fu, Chao-Ying; gcc-patches@gcc.gnu.org; Lau, David; 
> rdsandiford@googlemail.com
> Subject: Re: [PATCH] MIPS function attributes for interrupt handlers
> 
> Richard Sandiford wrote:
> 
> > 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.
> 
> FWIW, I tend to agree. 

The AVR port has a "naked" attribute available which, in rare circumstances, can still be useful. Probably because the AVR is an 8-bit micro, and can be very code constrained. This, of course, does not compare to something like the MIPS port.

Re: [PATCH] MIPS function attributes for interrupt handlers

[Mark Mitchell]

Weddington, Eric wrote:

> The AVR port has a "naked" attribute available which, in rare circumstances, can still be useful. Probably because the AVR is an 8-bit micro, and can be very code constrained. This, of course, does not compare to something like the MIPS port.

It turns out that the GCC manual already says that the only thing you
can put in a naked function are asm statements without operands.

Given that constraint, I have to adjust my position.  I think that given
that we've already constrained it that well, we might well allow this on
all ports.  I don't see any reason why it should be impossible to
support that on all architectures.  If we don't presently enforce the
constraint, we certainly can; it's an easy thing for the front-ends to
check.

Richard, thoughts?

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: [PATCH] MIPS function attributes for interrupt handlers

[Richard Sandiford]

Mark Mitchell <mark@codesourcery.com> writes:
> Weddington, Eric wrote:
>
>> The AVR port has a "naked" attribute available which, in rare circumstances, can still be useful. Probably because the AVR is an 8-bit micro, and can be very code constrained. This, of course, does not compare to something like the MIPS port.
>
> It turns out that the GCC manual already says that the only thing you
> can put in a naked function are asm statements without operands.
>
> Given that constraint, I have to adjust my position.  I think that given
> that we've already constrained it that well, we might well allow this on
> all ports.  I don't see any reason why it should be impossible to
> support that on all architectures.  If we don't presently enforce the
> constraint, we certainly can; it's an easy thing for the front-ends to
> check.
>
> Richard, thoughts?

I still disagree, for the reasons given before.  MIPS assembly coders
already have preprocessor macros to prettify the assembly function
declaration syntax.  If the only point of the attribute is to write
out that syntax, I don't think the attribute is a win.

(The reason the manual says what it does is that people used to try
to use operands with naked function asms, and tripped over problems.
The "asm with no operands" rule is a recent change (2008-05-23) and
hasn't yet appeared in any released compiler, so we don't really know
yet what effect the restriction has on real-world uses.  I bet the
version of gcc that the MIPS attribute was written for didn't enforce
this constraint.)

I don't think we've ever written down how the attribute interacts
with other features, such as function profiling.

Richard

Re: [PATCH] MIPS function attributes for interrupt handlers

[Mark Mitchell]

Richard Sandiford wrote:

> I still disagree, for the reasons given before.  MIPS assembly coders
> already have preprocessor macros to prettify the assembly function
> declaration syntax.  If the only point of the attribute is to write
> out that syntax, I don't think the attribute is a win.

I disagree on that point.  Assembly function declaration syntax is
complex, and varies from OS to OS, and depends on things like whether
the function is hidden, static, etc.  Making this an
architecture-independent attribute seems beneficial, as one of GCC's
core advantages is cross-platform consistency.

For example, writing:

  void mylock()
    __attribute__((hidden))
    __attribute__((naked)) {
    ARCH_MYLOCK;
  }

where ARCH_MYLOCK is a macro that expands to the CPU-specific assembly
implementation of a locking primitive seems likely to be useful.  Much
better than lots of #ifdef __linux__ and #ifdef __mips__ goo in a pure
assembler file.

> (The reason the manual says what it does is that people used to try
> to use operands with naked function asms, and tripped over problems.
> The "asm with no operands" rule is a recent change (2008-05-23) and
> hasn't yet appeared in any released compiler, so we don't really know
> yet what effect the restriction has on real-world uses.  I bet the
> version of gcc that the MIPS attribute was written for didn't enforce
> this constraint.)

Certainly so, but I'm not sure how this is relevant.  Is your point that
the attribute with the restrictions might not meet the intended use by
whoever first started using it?  If so, I agree -- but independent of
the original users, I still think it seems like a useful attribute.

> I don't think we've ever written down how the attribute interacts
> with other features, such as function profiling.

Indeed, though I think it's implied: if you don't write the right
assembly goo, you won't have profiling, since everything within the body
of the function is your responsibility.  I'm all for making it more
explicit, though.

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: [PATCH] MIPS function attributes for interrupt handlers

[Thiemo Seufer]

Mark Mitchell wrote:
> Richard Sandiford wrote:
> 
> > I still disagree, for the reasons given before.  MIPS assembly coders
> > already have preprocessor macros to prettify the assembly function
> > declaration syntax.  If the only point of the attribute is to write
> > out that syntax, I don't think the attribute is a win.
> 
> I disagree on that point.  Assembly function declaration syntax is
> complex, and varies from OS to OS, and depends on things like whether
> the function is hidden, static, etc.  Making this an
> architecture-independent attribute seems beneficial, as one of GCC's
> core advantages is cross-platform consistency.
> 
> For example, writing:
> 
>   void mylock()
>     __attribute__((hidden))
>     __attribute__((naked)) {
>     ARCH_MYLOCK;
>   }
> 
> where ARCH_MYLOCK is a macro that expands to the CPU-specific assembly
> implementation of a locking primitive seems likely to be useful.  Much
> better than lots of #ifdef __linux__ and #ifdef __mips__ goo in a pure
> assembler file.

The ifdef goo goes typically in the implementation of ARCH_MYLOCK,
declaring function entry/attributes etc. is easy to hide in preprocessor
macros.

AFAICS the best practical reason for __attribute__((naked)) is that it
allows to stick a mix of C and assembler which logically belongs together
in the same file. Currently the implementation of such code must be split
in a non-intuitive manner.


Thiemo

Re: [PATCH] MIPS function attributes for interrupt handlers

[Mark Mitchell]

Thiemo Seufer wrote:

> The ifdef goo goes typically in the implementation of ARCH_MYLOCK,
> declaring function entry/attributes etc. is easy to hide in preprocessor
> macros.

Yes, but why should you have to know that stuff?  Why not let the
compiler do it for you?

> AFAICS the best practical reason for __attribute__((naked)) is that it
> allows to stick a mix of C and assembler which logically belongs together
> in the same file.

Yes, that is a benefit.  In such situations, it also allows you to make
functions static that might otherwise have to be non-static.

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: [PATCH] MIPS function attributes for interrupt handlers

[Thiemo Seufer]

Mark Mitchell wrote:
> Thiemo Seufer wrote:
> 
> > The ifdef goo goes typically in the implementation of ARCH_MYLOCK,
> > declaring function entry/attributes etc. is easy to hide in preprocessor
> > macros.
> 
> Yes, but why should you have to know that stuff?  Why not let the
> compiler do it for you?

It is a trivial one-time effort which doesn't compare to writing correct
assembler code. So this argument is rather weak, IMHO.


Thiemo

Re: [PATCH] MIPS function attributes for interrupt handlers

[Mark Mitchell]

Thiemo Seufer wrote:

>>> The ifdef goo goes typically in the implementation of ARCH_MYLOCK,
>>> declaring function entry/attributes etc. is easy to hide in preprocessor
>>> macros.

> It is a trivial one-time effort which doesn't compare to writing correct
> assembler code. So this argument is rather weak, IMHO.

I've done it, I didn't find it trivial, and I didn't try to handle the
full generality of CPUs, OSes, ABIs, and so forth.  I guess we're just
going to have to agree to disagree.

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

RE: [PATCH] MIPS function attributes for interrupt handlers

[Weddington, Eric]

 

> -----Original Message-----
> From: Mark Mitchell [mailto:mark@codesourcery.com] 
> Sent: Tuesday, October 21, 2008 6:24 PM
> To: Thiemo Seufer
> Cc: Weddington, Eric; gcc-patches@gcc.gnu.org; 
> rdsandiford@googlemail.com
> Subject: Re: [PATCH] MIPS function attributes for interrupt handlers
> 
> Thiemo Seufer wrote:
> 
> >>> The ifdef goo goes typically in the implementation of ARCH_MYLOCK,
> >>> declaring function entry/attributes etc. is easy to hide 
> in preprocessor
> >>> macros.
> 
> > It is a trivial one-time effort which doesn't compare to 
> writing correct
> > assembler code. So this argument is rather weak, IMHO.
> 
> I've done it, I didn't find it trivial, and I didn't try to handle the
> full generality of CPUs, OSes, ABIs, and so forth.  I guess we're just
> going to have to agree to disagree.

The AVR port is a weird case. We rarely, if ever, have OSes and certainly nothing the size of Linux. There have been only two reasons why AVR users have actually used "naked" ISRs: one is to reduce the size of the generated pro/epilogue, because the AVR port pushes/pops more registers than is generally necessary. I agree that this is a failing of the AVR port and should be fixed, but for one reason or another, it hasn't gotten fixed. The other reason is that some users want to instrument the ISR in some way, before/after the pro/epilogue. I note that both cases are rare. Yes, our users could have written these ISRs in straight assembly, but sometimes they find it easier to create the function shell in C and write inline assembly. To each their own. I do know that in these rare cases, the "naked" attribute comes in handy. And yes, our users are fully cautioned about the hazards of using it.

Whether the MIPS port should have a "naked" attribute or not, is up to the MIPS maintainers and community. The MIPS and AVR ports are (obviously) very different. There are existing attributes that are port-specific in their semantics and implementation (e.g. "signal", "interrupt"). I just would have thought that "naked" would be left up to each port, whether to implement or not, and how to implement. I am just concerned about the statement that the "naked" attribute should not exist for *any* port. 

Re: [PATCH] MIPS function attributes for interrupt handlers

[Richard Sandiford]

Mark Mitchell <mark@codesourcery.com> writes:
> Richard Sandiford wrote:
>> I still disagree, for the reasons given before.  MIPS assembly coders
>> already have preprocessor macros to prettify the assembly function
>> declaration syntax.  If the only point of the attribute is to write
>> out that syntax, I don't think the attribute is a win.
>
> I disagree on that point.  Assembly function declaration syntax is
> complex, and varies from OS to OS, and depends on things like whether
> the function is hidden, static, etc.  Making this an
> architecture-independent attribute seems beneficial, as one of GCC's
> core advantages is cross-platform consistency.
>
> For example, writing:
>
>   void mylock()
>     __attribute__((hidden))
>     __attribute__((naked)) {
>     ARCH_MYLOCK;
>   }
>
> where ARCH_MYLOCK is a macro that expands to the CPU-specific assembly
> implementation of a locking primitive seems likely to be useful.  Much
> better than lots of #ifdef __linux__ and #ifdef __mips__ goo in a pure
> assembler file.

I'm not convinced by this example.  GCC already provides a much
more powerful way of doing this: extended asms.  Such an extended
asm has many benefits over naked functions:

  - It can be inlined into bigger functions.  That way, locks don't
    need to be function calls.

  - Other GCC features like function profiling continue to work correctly.

  - There is no need for an attribute.  GCC can tell what resources the
    asm needs, so when optimisation is enabled, GCC won't generate a
    prologue unless the asm needs one.

  - GCC can emit asm directives that depend on the code itself.
    Take the MIPS .frame pseudo-op as an example.  GCC doesn't
    know how much stack space a naked asm uses (if any), or what
    GPRs it uses, so it can't output a correct .frame for it.
    It _can_ (and does) output correct .frames for functions
    containing extended asms.

  - The extended-asm approach is _genuinely_ consistent across platforms.
    All ports support extended asms, and have done for many GCC releases.

    On the other hand, if we did decide to extend the naked attribute to
    other ports, it's likely to be several releases at least before it
    is genuinely consistent.  People would need to know which version of
    GCC first supported the attribute on the targets they care about.

AIUI, the only "advantage" of naked asms over extended asms is that they
allow you to do things that you wouldn't normally be allowed to do, like
write directly to the result register.  But why not simply bind the
return value to an output operand in an extended asm?  Is that
really much uglier than writing "__attribute__((naked))"?

Also, because naked functions cannot refer to their arguments, you will
get a warning for C unless you write:

  void __attribute__((naked))
  foo (int arg1 __attribute__((unused)), 
       int arg2 __attribute__((unused)))
  {
    ...;
  }

The macro definitions themselves would presumably look like:

  #define LOAD_AND_ADD_1 asm ("\
     lw $2,($4)\
     addiu $2,$2,1\
  ")

The combination of these two doesn't seem any prettier or easier
to me than having a source code file:

  FUNC_START(foo)
    lw $2,($4)
    addiu $2,$2,1
  FUNC_END(foo)

(which among other things avoids those infernal backslashes).

I agree with what Thiemo said about defining these assembly macros.
You said in reply that they were hard to write, but the point is that,
for the ports we're talking about, you don't need to.  Other people
already have.  (And most of the architectural differences between
these macros are historical.  GAS itself is much more consistent.)

It just seems to me that we're trying to introduce a feature in the name
of cross-platform consistency in cases where (a) in the foreseeable
future, "cross-platform" will mean "across a handful of targets" and
(b) we already have a better feature that is genuinely cross-platform.
(I say "foreseeable future" because we can't of course predict what
people will contribute.)

Richard

Re: [PATCH] MIPS function attributes for interrupt handlers

[Mark Mitchell]

Richard Sandiford wrote:

>> For example, writing:
>>
>>   void mylock()
>>     __attribute__((hidden))
>>     __attribute__((naked)) {
>>     ARCH_MYLOCK;
>>   }
>>
>> where ARCH_MYLOCK is a macro that expands to the CPU-specific assembly
>> implementation of a locking primitive seems likely to be useful.  Much
>> better than lots of #ifdef __linux__ and #ifdef __mips__ goo in a pure
>> assembler file.
> 
> I'm not convinced by this example.  GCC already provides a much
> more powerful way of doing this: extended asms.  Such an extended
> asm has many benefits over naked functions:

Sure, but it has one serious drawback: you can't use a non-standard
calling convention.

> I agree with what Thiemo said about defining these assembly macros.
> You said in reply that they were hard to write, but the point is that,
> for the ports we're talking about, you don't need to.

I'm sure they exist for MIPS GNU/Linux, for example.  But, I doubt they
exist for all architectures and all operating systems targeted by GCC,
in some convenient location where everyone can find them, under
licensing terms that everyone can use.

My argument here is that we have an opportunity to move towards making
this an architecture-independent feature.  But, if you block that
direction on MIPS, then we can't make it architecture-independent.
Whether or not the feature is most useful on MIPS or not is not the
point; the point is that it may be useful on many architectures, and
having MIPS be consistent is advantageous.

I feel the same way about the interrupt attribute.  Even on machines
where there is no special interrupt calling convention, supporting the
interrupt attribute -- and spelling it in the same way -- is useful,
since it allows people to write interrupt handlers without
conditionalizing for that.

> It just seems to me that we're trying to introduce a feature in the name
> of cross-platform consistency in cases where (a) in the foreseeable
> future, "cross-platform" will mean "across a handful of targets" and
> (b) we already have a better feature that is genuinely cross-platform.
> (I say "foreseeable future" because we can't of course predict what
> people will contribute.)

GCC is always incremental; it's relatively rare that something that
requires target support appears in all ports at once.  So, I agree that
it would take time to get consistency, but I don't see why that's a good
reason not to put this attribute into more ports.

If we really think this attribute is so harmful, we should be
deprecating it and removing it from other ports.  But, as defined now,
it doesn't seem linguistically problematic, and there is a use case for
it.

What is the harm in allowing it for MIPS?

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

Re: [PATCH] MIPS function attributes for interrupt handlers

[Richard Sandiford]

Mark Mitchell <mark@codesourcery.com> writes:
> Richard Sandiford wrote:
>>> For example, writing:
>>>
>>>   void mylock()
>>>     __attribute__((hidden))
>>>     __attribute__((naked)) {
>>>     ARCH_MYLOCK;
>>>   }
>>>
>>> where ARCH_MYLOCK is a macro that expands to the CPU-specific assembly
>>> implementation of a locking primitive seems likely to be useful.  Much
>>> better than lots of #ifdef __linux__ and #ifdef __mips__ goo in a pure
>>> assembler file.
>> 
>> I'm not convinced by this example.  GCC already provides a much
>> more powerful way of doing this: extended asms.  Such an extended
>> asm has many benefits over naked functions:
>
> Sure, but it has one serious drawback: you can't use a non-standard
> calling convention.

Is that really an issue?  You wouldn't be able to call the function
from GCC in that case either, so why tell GCC about it at all?

> What is the harm in allowing it for MIPS?

I think at this point we're going round in circles ;)  I don't think
I can explain it any better than I already have.

Richard

Re: [PATCH] MIPS function attributes for interrupt handlers

[Fu, Chao-Ying]

Richard Sandiford wrote:
>
> "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.
>

  Since James left Microchip, I tried to contact his manager (Mike Collison)
at Microchip.  His reply is attached below.  Thanks!

Regards,
Chao-ying

> -----Original Message-----
> From: Mike.Collison@microchip.com [mailto:Mike.Collison@microchip.com]
> Sent: Monday, October 27, 2008 3:26 PM
> To: Fu, Chao-Ying
> Cc: Anderton, Ian
> Subject: RE: Questions about GCC
>
>
>
> Hi Chao-ying,
>
> Sorry for getting back to you so late but I needed to verify our
> copyright assignment. I can report that we do have a current copyright
> assignment in place with the FSF.
>
> Michael Collison
> Manager, Language Tools
> Office - (480) 792-4318
>
> -----Original Message-----
> From: Fu, Chao-Ying [mailto:fu@mips.com]
> Sent: Tuesday, October 21, 2008 2:59 PM
> To: Mike Collison - C12745
> Cc: Anderton, Ian
> Subject: Questions about GCC
>
> Hi Mike,
>
>   This is Chao-ying Fu from MIPS Technologies Inc.  I got your
> contact from Ian Anderton.  The reason that I contacted you is as
> follows.
>
>   MIPS wants to implement function attributes for interrupt handlers
> into FSF GCC mainline (4.4).  Since Microchip PIC32 GCC
> already has this
> feature in
> GCC 3.4, it will be very helpful to reuse some of your source code.
> The issue is that we need to know if you have copyright assignment to
> FSF before.
> If not, do you mind submitting copyright assignment to FSF?
> http://gcc.gnu.org/contribute.html
> We will port some code to FSF GCC mainline, and this may help both
> Microchip and
> MIPS to migrate toolchains to the latest version easily in the future.
>
>   Thanks a lot!
>
> Regards,
> Chao-ying Fu
> MIPS Technologies Inc

Re: [PATCH] MIPS function attributes for interrupt handlers

[Richard Sandiford]

"Fu, Chao-Ying" <fu@mips.com> writes:
> Richard Sandiford wrote:
>>
>> "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.
>>
>
>   Since James left Microchip, I tried to contact his manager (Mike Collison)
> at Microchip.  His reply is attached below.

Excellent, thanks.

Richard

Re: [PATCH] MIPS function attributes for interrupt handlers

[Fu, Chao-Ying]

Hi All,

  Based on the discussions last year, I will update my interrupt patch to
support only the following
attributes.

void __attribute__ ((interrupt )) v0 () { }
void __attribute__ ((interrupt (use_shadow_register_set))) v1 () { }
void __attribute__ ((interrupt (keep_interrupts_masked))) v2 () { }
void __attribute__ ((interrupt (use_shadow_register_set,
keep_interrupts_masked))) v3 () { }

  The default "interrupt" attribute will generate code that doesn't use
shadow register sets,
and doesn't keep interrupts masked.

  The previous "naked", "vector", "at_vector" attributes will be removed
from the patch, because
there are issues about the usage of "naked", and "vector"/"at_vector"
actually just put the functions
into named sections (not the memory address of vectors).

 Are people ok with attributes and the names?  Thanks!

Regards,
Chao-ying

Re: [PATCH] MIPS function attributes for interrupt handlers

[Adam Nemet]

Fu, Chao-Ying writes:
> void __attribute__ ((interrupt )) v0 () { }
> void __attribute__ ((interrupt (use_shadow_register_set))) v1 () { }
> void __attribute__ ((interrupt (keep_interrupts_masked))) v2 () { }
> void __attribute__ ((interrupt (use_shadow_register_set,
> keep_interrupts_masked))) v3 () { }
> 
>   The default "interrupt" attribute will generate code that doesn't use
> shadow register sets,
> and doesn't keep interrupts masked.
> 
>   The previous "naked", "vector", "at_vector" attributes will be removed
> from the patch, because
> there are issues about the usage of "naked", and "vector"/"at_vector"
> actually just put the functions
> into named sections (not the memory address of vectors).
> 
>  Are people ok with attributes and the names?  Thanks!

Thanks for incorporating my comments.  FWIW, I am fine with these.

Adam

Re: [PATCH] MIPS function attributes for interrupt handlers

[Daniel Jacobowitz]

On Tue, Feb 24, 2009 at 06:02:08PM -0800, Fu, Chao-Ying wrote:
> Hi All,
> 
>   Based on the discussions last year, I will update my interrupt patch to
> support only the following
> attributes.
> 
> void __attribute__ ((interrupt )) v0 () { }
> void __attribute__ ((interrupt (use_shadow_register_set))) v1 () { }
> void __attribute__ ((interrupt (keep_interrupts_masked))) v2 () { }
> void __attribute__ ((interrupt (use_shadow_register_set,
> keep_interrupts_masked))) v3 () { }

I have one question: does the patch cause GCC to generate eret?  If
so, is there another attribute needed for deret?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] MIPS function attributes for interrupt handlers

[Fu, Chao-Ying]

"Daniel Jacobowitz" wrote:
> On Tue, Feb 24, 2009 at 06:02:08PM -0800, Fu, Chao-Ying wrote:
> > Hi All,
> >
> >   Based on the discussions last year, I will update my interrupt patch
to
> > support only the following
> > attributes.
> >
> > void __attribute__ ((interrupt )) v0 () { }
> > void __attribute__ ((interrupt (use_shadow_register_set))) v1 () { }
> > void __attribute__ ((interrupt (keep_interrupts_masked))) v2 () { }
> > void __attribute__ ((interrupt (use_shadow_register_set,
> > keep_interrupts_masked))) v3 () { }
>
> I have one question: does the patch cause GCC to generate eret?  If
> so, is there another attribute needed for deret?

  Yes, from the current patch GCC generates "eret" for return.
So, we may need another attribute as "use_debug_exception_return".
Thanks!

Regards,
Chao-ying

Re: [PATCH] MIPS function attributes for interrupt handlers

[Maciej W. Rozycki]

On Wed, 25 Feb 2009, Fu, Chao-Ying wrote:

> > I have one question: does the patch cause GCC to generate eret?  If
> > so, is there another attribute needed for deret?
> 
>   Yes, from the current patch GCC generates "eret" for return.
> So, we may need another attribute as "use_debug_exception_return".

 Please note that "eret" is not supported for -march=mips1 and 
-march=mips2 in GAS as those ISAs did not have this instrution ("rfe" in 
the delay slot of a "jr" was used instead).  While, if I understand 
correctly, the interrupt function attribute makes sense for the EIC mode 
only, I suggest that you make sure the compiler does not produce code GAS 
will refuse to swallow for older ISAs.  Perhaps the attribute should only 
be supported for ISAs which can actually run the resulting code 
("-march=mips32" and above?).

 For portable kernel code which might be built with different ISA options 
(to be run on processors implementing different ISAs, e.g. -march=mips3 to 
run on all 64-bit hardware) this would force people to keep interrupt 
handlers in separate source files always build with the right -march= 
option.  I suppose that would be a bearable solution.

  Maciej

Re: [PATCH] MIPS function attributes for interrupt handlers

[Fu, Chao-Ying]

"Maciej W. Rozycki" wrote:
> On Wed, 25 Feb 2009, Fu, Chao-Ying wrote:
>
> > > I have one question: does the patch cause GCC to generate eret?  If
> > > so, is there another attribute needed for deret?
> >
> >   Yes, from the current patch GCC generates "eret" for return.
> > So, we may need another attribute as "use_debug_exception_return".
>
>  Please note that "eret" is not supported for -march=mips1 and
> -march=mips2 in GAS as those ISAs did not have this instrution ("rfe" in
> the delay slot of a "jr" was used instead).  While, if I understand
> correctly, the interrupt function attribute makes sense for the EIC mode
> only, I suggest that you make sure the compiler does not produce code GAS
> will refuse to swallow for older ISAs.  Perhaps the attribute should only
> be supported for ISAs which can actually run the resulting code
> ("-march=mips32" and above?).

  It can be used for the original mode (single,ilp0.. ilp7) and the EIC
mode.
People just need  to put GCC-generated handlers to the address they want
in the linker script, and provide some bootup code.

  Maybe we just emit "jr" and "ref", when mips1 or mips2 is used to compile
code.
Thanks!

Regards,
Chao-ying

Re: [PATCH] MIPS function attributes for interrupt handlers

[Maciej W. Rozycki]

On Fri, 27 Feb 2009, Fu, Chao-Ying wrote:

> >  Please note that "eret" is not supported for -march=mips1 and
> > -march=mips2 in GAS as those ISAs did not have this instrution ("rfe" in
> > the delay slot of a "jr" was used instead).  While, if I understand
> > correctly, the interrupt function attribute makes sense for the EIC mode
> > only, I suggest that you make sure the compiler does not produce code GAS
> > will refuse to swallow for older ISAs.  Perhaps the attribute should only
> > be supported for ISAs which can actually run the resulting code
> > ("-march=mips32" and above?).
> 
>   It can be used for the original mode (single,ilp0.. ilp7) and the EIC
> mode.
> 
> People just need  to put GCC-generated handlers to the address they want
> in the linker script, and provide some bootup code.

 Hmm, I thought the shadow register sets could only be switched 
automatically with the EIC mode.  I have checked the ISA doc now to be 
proved I was slightly confused -- the R2 ISA retrofitted the SRS switching 
into the older concept of the use of a dedicated exception vector (as 
provided by cp0.cause.iv) for the interrupt exception.

 All is a new MIPS32r2/MIPS64r2 addition anyway.  With older processors 
you have to perform stack switching in software and then save/restore 
registers on the interrupt stack and I think that's too system-specific to 
be doable from the compiler in a generic way.

>   Maybe we just emit "jr" and "ref", when mips1 or mips2 is used to compile
> code.

 IMO it does not make sense to support it in any way except from making a 
small amount of effort to make sure such configurations do not make the 
tools behave inconsistently.  There is no MIPS I or II processor that 
would support the SRS so the interrupt handler won't ever run on one.

  Maciej

Re: [PATCH] MIPS function attributes for interrupt handlers

[Ross Ridge]

Mark Mitchell writes:
>It turns out that the GCC manual already says that the only thing you
>can put in a naked function are asm statements without operands.
>
>Given that constraint, I have to adjust my position.  I think that given
>that we've already constrained it that well, we might well allow this on
>all ports.  I don't see any reason why it should be impossible to
>support that on all architectures.  If we don't presently enforce the
>constraint, we certainly can; it's an easy thing for the front-ends to
>check.

I think that's unecessarily too constraining.  I prefer your earlier
suggestion that "any inputs, outputs, clobbers, etc. contain only
references to entities with static storage duration."

In order to support callbacks from an assembly based API where arguments
are passed using abritrary registers and flags, I used the naked
attribute to implement "thunk" wrapper functions.  The thunks where
created by macros that expanded to a naked function containing an asm
statement with a single operand, the callback function being wrapped.
The functions created by the macros were of the form:

	/* extern int callback(int); */

	void __attribute__((naked)) __attribute__((section("_LTEXT"))
	_cbthunk_callback(void) {
		asm volatile("pushl %%edi\n\t"
			     "call %P0\n\t"
			     "addl $4,%%esp\n\t"
			     "negl %%eax\n\t" /* set carry from EAX */
			     "ret" : : "g" (callback));
	}

This saved from me having to know how "callback" was spelt as assembly
name, and gave compile time errors if I mispelt "callback" instead of
link time errors.

I don't see why asm statements in naked functions couldn't support
operands containing either a constant value or entity with a constant
address that's supported in asm statements elsewhere.

					Ross Ridge

RE: [PATCH] MIPS function attributes for interrupt handlers

[Fu, Chao-Ying]

Nemet wrote:
> > +
> > +      /* 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) */
> > +	  fprintf (file, "\trdpgpr\t$sp, $sp\n");
> 
> You seems to be using MIPS32r2 instructions in the prologue.  
> I think this should be emitted in RTL.

  Ok.

> It seems that ipl7 implies two things: use the shadow 
> register set and that all interrupts should remain masked.  
> It seems the second can be useful even if the processor does 
> not support shadow registers.  One idea would be to split the 
> two and make them explicit (e.g. use_shadow_register_set and 
> keep_interrupts_masked attributes).
> 

  Yes. You are right.  Single and ipl0-ipl6 generate
same code that doesn't use shadow register sets, and allows nested
interrupts.  Ipl7 uses a shadow register set and doesn't allow
nested interrupts.
Using your two attributes allow a new combination.

> > +@item at_vector
> > +@cindex install interrupt handler at the address of 
> exception vector 
> > +on MIPS Using this attribute on MIPS indicates that the specific 
> > +interrupt handler should be located at the address of the 
> exception 
> > +vector (from 0 to 63).
> > +
> > +@smallexample
> > +void f ()  __attribute__ ((interrupt(single))) 
> > +__attribute__((at_vector (20))); @end smallexample
> > +
> > +The single interrupt handler f is installed at the 
> exception vectors 
> > +20 which section name is ".vector_20".
> 
> I think we shouldn't suggest here that we install this 
> function at a certain address but be clear that all this does 
> is to put the function into a named section.

  Ok.

Regards,
Chao-ying