Re: [PATCH : RL78] Disable interrupts during hardware multiplication routines

[Nick Clifton <nickc at redhat dot com>]

Hi Kaushik,

  Just a few comments on the patch itself:

+/* Check if the block uses mul/div insns.  */
+int
+check_mduc_usage ()

I would suggest:

  static bool
  check_mduc_usage (void)

instead, since this is a boolean function, only used in the rl78.c file, 
and it takes no arguments.


+      if (recog_memoized (insn) == CODE_FOR_udivmodsi4_g13
+          || recog_memoized (insn) == CODE_FOR_mulhi3_g13
+          || recog_memoized (insn) == CODE_FOR_mulsi3_g13)

Testing for specific insn codes is a bit unsafe as it would lead to 
problems if new multiply insns are ever added to the target.  A slightly 
better approach would be to have an attribute on these insns, and then 
test for that here.


+          return 1;

If you change the function to "bool" then return "true" here.

+  return 0;

And "false" here.


   if (rl78_is_naked_func ())
     return;
-
+

Why are you adding a extraneous space here ?


+  /* Save MDUC register inside interrupt routine.  */
+  if (MUST_SAVE_MDUC_REGISTER && (!crtl->is_leaf || check_mduc_usage ()))

It would be nice to update the stack size computation performed earlier 
in this function, if these registers are going to be saved.


+    {
+      rtx mem_mduc;
+
+      mem_mduc = gen_rtx_MEM (QImode, GEN_INT (0xf00e8));
+      MEM_VOLATILE_P (mem_mduc) = 1;
+      emit_insn (gen_movqi (gen_rtx_REG (QImode, A_REG), mem_mduc));
+      emit_insn (gen_push (gen_rtx_REG (HImode, AX_REG)));
+
+      mem_mduc = gen_rtx_MEM (HImode, GEN_INT (0xffff0));
+      MEM_VOLATILE_P (mem_mduc) = 1;
+      emit_insn (gen_movhi (gen_rtx_REG (HImode, AX_REG), mem_mduc));
+      emit_insn (gen_push (gen_rtx_REG (HImode, AX_REG)));

Since this is a lot of repeated code, it would be cleaner to use a for 
loop and a small structure containing the address, mode and volatility 
to be saved.  Even better this structure could then be shared with the 
epilogue code so that the two functions always remain in sync.


   if (cfun->machine->uses_es)
     fprintf (file, "\t; uses ES register\n");
+
+  if (MUST_SAVE_MDUC_REGISTER)
+    fprintf (file, "\t; uses MDUC register\n");
 }

It is not just that the function uses the MDUC registers.  It is also 
that there registers are saved on the stack - and hence the function 
uses more stack space than the reader might expect.  Hence I would suggest:

  fprintf (file, "\t; preserves MDUC registers\n");


+msave-mduc-in-interrupts
+Target Mask(SAVE_MDUC_REGISTER)
+Stores the MDUC registers for interrupt handlers for G13 target.
+This is the default.

It is *not* the default!


+@item -msave-mduc-in-interrupts
+@item -mno-save-mduc-in-interrupts
+@opindex msave-mduc-in-interrupts
+@opindex mno-save-mduc-in-interrupts
+Specifies that interrupt handler functions should preserve the
+MDUC registers.  This is only necessary if normal code might use
+the MDUC registers, for example because it performs multiplication
+and division operations. The default is to ignore the MDUC registers
+as this makes the interrupt handlers faster. The target option -mg13
+needs to be passed for this to work as this feature is only available
+on the G13 target (S2 core).

You should also mention that even if this option is enabled the MDUC 
registers will not be saved if the interrupt function does not use the 
multiply hardware and does not call any other function.



Cheers
  Nick