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

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

[Kaushik M Phatak]

 Hi Mike,
Thanks for the feedback regarding the ABI.

I have added Nick and DJ in the loop to see if they can review this
patch posted last month,
https://gcc.gnu.org/ml/gcc-patches/2015-12/msg00733.html

Best Regards,
Kaushik


On Wed, Dec 9, 2015 at 10:23 PM, Mike Stump <mikestump@comcast.net> wrote:
> On Dec 9, 2015, at 1:53 AM, Kaushik M Phatak <kphatak@gmail.com> wrote:
>> Thank you for your feedback on my patch. (27 August 2015)
>> I have posted an updated patch for this issue,
>> https://gcc.gnu.org/ml/gcc-patches/2015-12/msg00733.html
>>
>> To reply to your question, the RL78 ABI does not specify anything for the MDUC
>> registers (Multiply/Divide).
>
> Three options then, either you're inventing the abi, then you decide and document what you did, you ask someone else what the abi is, or you follow the abi set by another compiler.  You cannot escape the abi.  As port contributor, you get to pick and make the best choice you can.  If you cannot, then I will pick, the registers are always saved.  Correspondingly, they are free to code-gen to be used by the compiler and optimizer.  If that's the case, then there should be no flag.  An os (aka target) that wants a different choice will responsible for adding in the code that can do something else.
>
> Let us know what your choice is.   Maybe Nick knows of a reason why the abi should be a certain way, Nick?  One we know the choice, we can then judge the patch to see if it implements the abi.

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

[Nick Clifton]

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

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

[Kaushik M Phatak]

[-- Attachment #1: Type: text/plain, Size: 11064 bytes --]

Hi Nick,
Thanks for your detailed review.
Please find an updated version of this patch here. I have tried to modify
it as per your suggestions.

> I would suggest:
>  static bool
Done.

> +      if (recog_memoized (insn) == CODE_FOR_udivmodsi4_g13
> have an attribute on these insns, and then test for that here.
Done. Added attribute 'is_g13_muldiv_insn' which gets tested in this function.

> +          return 1;
> If you change the function to "bool" then return "true" here.
> +  return 0;
Done.

>    if (rl78_is_naked_func ())
>     return;
> -
> +
> Why are you adding a extraneous space here ?
Done. This was a typo while extracting the patch.

> +  if (MUST_SAVE_MDUC_REGISTER && (!crtl->is_leaf || check_mduc_usage ()))
> It would be nice to update the stack size computation performed
I have tried to add code to update 'fs' while checking for 'framesize_locals'.
Please let me know if this is OK.

> +      emit_insn (gen_push (gen_rtx_REG (HImode, AX_REG)));
> it would be cleaner to use a for loop and a small structure containing
> the address, mode and volatility to be saved.
I have created simple structure for this, though the volatility is always set
and may not need to be a separate member.
Please let me know if this is OK.

> Hence I would suggest:
> fprintf (file, "\t; preserves MDUC registers\n");
Done.

> +This is the default.
> It is *not* the default!
Updated this. However, the registers are saved when -mmul=g13 and
-mg13 is passed.

> +@item -msave-mduc-in-interrupts
> +@item -mno-save-mduc-in-interrupts
> +@opindex msave-mduc-in-interrupts
> You should also mention that even if this option is enabled
Added information regarding registers being saved based on certain
conditions being met.

Kindly review the updated patch and let me know if it is OK.
This is regression tested for rl78 -msim and -mg13 + -msave-mduc-in-interrupts

Best Regards,
Kaushik

gcc/ChangeLog
2016-01-12  Kaushik Phatak  <kaushik.phatak@kpit.com>

* config/rl78/rl78.c (rl78_expand_prologue): Save the MDUC related
registers in all interrupt handlers if necessary.
(rl78_option_override): Add warning.
(MUST_SAVE_MDUC_REGISTER): New macro.
(rl78_expand_epilogue): Restore the MDUC registers if necessary.
* config/rl78/rl78.c (check_mduc_usage): New function.
* config/rl78/rl78.c (mduc_regs): New structure to hold MDUC register data.
* config/rl78/rl78.md (is_g13_muldiv_insn): New attribute.
* config/rl78/rl78.md (mulsi3_g13): Add is_g13_muldiv_insn attribute.
* config/rl78/rl78.md (udivmodsi4_g13): Add is_g13_muldiv_insn attribute.
* config/rl78/rl78.md (mulhi3_g13): Add is_g13_muldiv_insn attribute.
* config/rl78/rl78.opt (msave-mduc-in-interrupts): New option.
(mno-save-mduc-in-interrupts): New option.
* doc/invoke.texi (@item -msave-mduc-in-interrupts): New item.
(@item -mno-save-mduc-in-interrupts): New item

Index: gcc/config/rl78/rl78.c
===================================================================
--- gcc/config/rl78/rl78.c(revision 2871)
+++ gcc/config/rl78/rl78.c(working copy)
@@ -83,6 +83,22 @@
   "sp", "ap", "psw", "es", "cs"
 };

+/* Structure for G13 MDUC registers.  */
+struct mduc_reg_type
+{
+  unsigned int address;
+  enum machine_mode mode;
+  bool is_volatile;
+};
+
+struct mduc_reg_type  mduc_regs[NUM_OF_MDUC_REGS] =
+  {{0xf00e8, QImode, true},
+   {0xffff0, HImode, true},
+   {0xffff2, HImode, true},
+   {0xf2224, HImode, true},
+   {0xf00e0, HImode, true},
+   {0xf00e2, HImode, true}};
+
 struct GTY(()) machine_function
 {
   /* If set, the rest of the fields have been computed.  */
@@ -342,6 +358,10 @@
 #undef  TARGET_OPTION_OVERRIDE
 #define TARGET_OPTION_OVERRIDErl78_option_override

+#define MUST_SAVE_MDUC_REGISTERS                         \
+  (!TARGET_NO_SAVE_MDUC_REGISTERS                        \
+   && (is_interrupt_func (NULL_TREE)) && RL78_MUL_G13)
+
 static void
 rl78_option_override (void)
 {
@@ -366,6 +386,9 @@
     /* Address spaces are currently only supported by C.  */
     error ("-mes0 can only be used with C");

+  if (TARGET_SAVE_MDUC_REGISTERS && !(TARGET_G13 || RL78_MUL_G13))
+    warning (0, "mduc registers only saved for G13 target");
+
   switch (rl78_cpu_type)
     {
     case CPU_UNINIT:
@@ -1307,6 +1330,23 @@
   return (lookup_attribute ("naked", DECL_ATTRIBUTES
(current_function_decl)) != NULL_TREE);
 }

+/* Check if the block uses mul/div insns for G13 target.  */
+static bool
+check_mduc_usage ()
+{
+  rtx insn;
+  basic_block bb;
+  FOR_EACH_BB_FN (bb, cfun)
+  {
+    FOR_BB_INSNS (bb, insn)
+    {
+      if (get_attr_is_g13_muldiv_insn (insn) == IS_G13_MULDIV_INSN_YES)
+        return true;
+    }
+  }
+  return false;
+}
+
 /* Expand the function prologue (from the prologue pattern).  */
 void
 rl78_expand_prologue (void)
@@ -1371,6 +1411,28 @@
       F (emit_insn (gen_push (ax)));
     }

+  /* Save MDUC registers inside interrupt routine.  */
+  if (MUST_SAVE_MDUC_REGISTERS && (!crtl->is_leaf || check_mduc_usage ()))
+    {
+      rtx mem_mduc;
+
+      for (int i = 0; i <NUM_OF_MDUC_REGS; i++)
+      {
+if (mduc_regs[i].mode == QImode)
+{
+  mem_mduc = gen_rtx_MEM (QImode, GEN_INT (mduc_regs[i].address));
+  MEM_VOLATILE_P (mem_mduc) = 1;
+  emit_insn (gen_movqi (gen_rtx_REG (QImode, A_REG), mem_mduc));
+}
+else
+{
+  mem_mduc = gen_rtx_MEM (HImode, GEN_INT (mduc_regs[i].address));
+  MEM_VOLATILE_P (mem_mduc) = 1;
+  emit_insn (gen_movqi (gen_rtx_REG (HImode, AX_REG), mem_mduc));
+}
+emit_insn (gen_push (gen_rtx_REG (HImode, AX_REG)));
+      }
+    }
   if (frame_pointer_needed)
     {
       F (emit_move_insn (ax, sp));
@@ -1378,6 +1440,8 @@
     }

   fs = cfun->machine->framesize_locals + cfun->machine->framesize_outgoing;
+  if (MUST_SAVE_MDUC_REGISTERS && (!crtl->is_leaf || check_mduc_usage ()))
+    fs = fs + NUM_OF_MDUC_REGS * 2;
   if (fs > 0)
     {
       /* If we need to subtract more than 254*3 then it is faster and
@@ -1426,6 +1490,8 @@
   else
     {
       fs = cfun->machine->framesize_locals + cfun->machine->framesize_outgoing;
+      if (MUST_SAVE_MDUC_REGISTERS && (!crtl->is_leaf || check_mduc_usage ()))
+fs = fs + NUM_OF_MDUC_REGS * 2;
       if (fs > 254 * 3)
 {
   emit_move_insn (ax, sp);
@@ -1444,6 +1510,29 @@
 }
     }

+  /* Restore MDUC registers from interrupt routine.  */
+  if (MUST_SAVE_MDUC_REGISTERS && (!crtl->is_leaf || check_mduc_usage ()))
+    {
+      rtx mem_mduc;
+
+      for (int i = NUM_OF_MDUC_REGS-1; i >= 0; i--)
+      {
+emit_insn (gen_pop (gen_rtx_REG (HImode, AX_REG)));
+        if (mduc_regs[i].mode == QImode)
+        {
+          mem_mduc = gen_rtx_MEM (QImode, GEN_INT (mduc_regs[i].address));
+          MEM_VOLATILE_P (mem_mduc) = 1;
+  emit_insn (gen_movqi (mem_mduc, gen_rtx_REG (QImode, A_REG)));
+        }
+        else
+        {
+          mem_mduc = gen_rtx_MEM (HImode, GEN_INT (mduc_regs[i].address));
+          MEM_VOLATILE_P (mem_mduc) = 1;
+  emit_insn (gen_movhi (mem_mduc, gen_rtx_REG (HImode, AX_REG)));
+        }
+      }
+    }
+
   /* Save ES register inside interrupt functions.  */
   if (is_interrupt_func (cfun->decl) && cfun->machine->uses_es)
     {
@@ -1599,6 +1688,9 @@

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

 /* Return an RTL describing where a function return value of type RET_TYPE
Index: gcc/config/rl78/rl78.h
===================================================================
--- gcc/config/rl78/rl78.h(revision 2871)
+++ gcc/config/rl78/rl78.h(working copy)
@@ -28,6 +28,7 @@
 #define TARGET_G14(rl78_cpu_type == CPU_G14)


+#define NUM_OF_MDUC_REGS 6

 #define TARGET_CPU_CPP_BUILTINS()    \
   do                                                \
Index: gcc/config/rl78/rl78.md
===================================================================
--- gcc/config/rl78/rl78.md(revision 2871)
+++ gcc/config/rl78/rl78.md(working copy)
@@ -78,6 +78,7 @@
 (include "rl78-virt.md")
 (include "rl78-real.md")

+(define_attr "is_g13_muldiv_insn" "yes,no" (const_string "no"))

 ;; Function Prologue/Epilogue Instructions

@@ -416,7 +417,8 @@
 movw    ax, 0xffff6     ; MDBL
 movw    %h0, ax
         ; end of mulhi macro"
-  [(set_attr "valloc" "macax")]
+  [(set_attr "valloc" "macax")
+   (set_attr "is_g13_muldiv_insn" "yes")]
 )

 ;; 0xFFFF0 is MACR(L).  0xFFFF2 is MACR(H) but we don't care about it
@@ -496,7 +498,8 @@
 movwax, !0xf00e0; MDCL
 movw%H0, ax
 ; end of mulsi macro"
-  [(set_attr "valloc" "macax")]
+  [(set_attr "valloc" "macax")
+   (set_attr "is_g13_muldiv_insn" "yes")]
 )

 ;; start-sanitize-rl78
@@ -731,7 +734,8 @@
 movw%H3, ax\n\
 ; end of udivmodsi macro";
       }
-  [(set_attr "valloc" "macax")]
+  [(set_attr "valloc" "macax")
+   (set_attr "is_g13_muldiv_insn" "yes")]
 )
 (define_insn "mov1"
   [(set (match_operand:QI         0 "rl78_nonimmediate_operand"   "=vU")
Index: gcc/config/rl78/rl78.opt
===================================================================
--- gcc/config/rl78/rl78.opt(revision 2871)
+++ gcc/config/rl78/rl78.opt(working copy)
@@ -103,4 +103,10 @@
 Target Mask(ES0)
 Assume ES is zero throughout program execution, use ES: for read-only data.

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

+mno-save-mduc-in-interrupts
+Target RejectNegative Mask(NO_SAVE_MDUC_REGISTERS)
+Does not save the MDUC registers in interrupt handlers for G13 target.
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi(revision 2871)
+++ gcc/doc/invoke.texi(working copy)
@@ -18901,6 +18901,20 @@
 registers @code{r24..r31} are reserved for use in interrupt handlers.
 With this option enabled these registers can be used in ordinary
 functions as well.
+
+@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). The option will not have any effect if
+the target does not have multiply hardware, or if the interrupt
+function does not call any other function.
 @end table

@node RS/6000 and PowerPC Options
================== END of Patch ==============

-----Original Message-----
From: Nick Clifton [mailto:nickc@redhat.com]
Sent: Thursday, January 07, 2016 5:33 PM
To: Kaushik M Phatak <kphatak@gmail.com>; dj@redhat.com
Cc: Mike Stump <mikestump@comcast.net>; gcc-patches@gcc.gnu.org;
Kaushik Phatak <Kaushik.Phatak@kpit.com>
Subject: Re: [PATCH : RL78] Disable interrupts during hardware
multiplication routines

Hi Kaushik,

   Just a few comments on the patch itself:

[-- Attachment #2: rl78_mduc_patch_2.diff --]
[-- Type: text/plain, Size: 7838 bytes --]

Index: gcc/config/rl78/rl78.c
===================================================================
--- gcc/config/rl78/rl78.c	(revision 2871)
+++ gcc/config/rl78/rl78.c	(working copy)
@@ -83,6 +83,22 @@
   "sp", "ap", "psw", "es", "cs"
 };
 
+/* Structure for G13 MDUC registers.  */
+struct mduc_reg_type
+{
+  unsigned int address;
+  enum machine_mode mode;
+  bool is_volatile;
+};
+
+struct mduc_reg_type  mduc_regs[NUM_OF_MDUC_REGS] =
+  {{0xf00e8, QImode, true},
+   {0xffff0, HImode, true},
+   {0xffff2, HImode, true},
+   {0xf2224, HImode, true},
+   {0xf00e0, HImode, true},
+   {0xf00e2, HImode, true}};
+
 struct GTY(()) machine_function
 {
   /* If set, the rest of the fields have been computed.  */
@@ -342,6 +358,10 @@
 #undef  TARGET_OPTION_OVERRIDE
 #define TARGET_OPTION_OVERRIDE		rl78_option_override
 
+#define MUST_SAVE_MDUC_REGISTERS                         \
+  (!TARGET_NO_SAVE_MDUC_REGISTERS                        \
+   && (is_interrupt_func (NULL_TREE)) && RL78_MUL_G13)
+
 static void
 rl78_option_override (void)
 {
@@ -366,6 +386,9 @@
     /* Address spaces are currently only supported by C.  */
     error ("-mes0 can only be used with C");
 
+  if (TARGET_SAVE_MDUC_REGISTERS && !(TARGET_G13 || RL78_MUL_G13))
+    warning (0, "mduc registers only saved for G13 target");
+
   switch (rl78_cpu_type)
     {
     case CPU_UNINIT:
@@ -1307,6 +1330,23 @@
   return (lookup_attribute ("naked", DECL_ATTRIBUTES (current_function_decl)) != NULL_TREE);
 }
 
+/* Check if the block uses mul/div insns for G13 target.  */
+static bool
+check_mduc_usage ()
+{
+  rtx insn;
+  basic_block bb;
+  FOR_EACH_BB_FN (bb, cfun)
+  {
+    FOR_BB_INSNS (bb, insn)
+    {
+      if (get_attr_is_g13_muldiv_insn (insn) == IS_G13_MULDIV_INSN_YES)
+        return true;
+    }
+  }
+  return false;
+}
+
 /* Expand the function prologue (from the prologue pattern).  */
 void
 rl78_expand_prologue (void)
@@ -1371,6 +1411,28 @@
       F (emit_insn (gen_push (ax)));
     }
 
+  /* Save MDUC registers inside interrupt routine.  */
+  if (MUST_SAVE_MDUC_REGISTERS && (!crtl->is_leaf || check_mduc_usage ()))
+    {
+      rtx mem_mduc;
+
+      for (int i = 0; i <NUM_OF_MDUC_REGS; i++)
+      {
+	if (mduc_regs[i].mode == QImode)
+	{
+	  mem_mduc = gen_rtx_MEM (QImode, GEN_INT (mduc_regs[i].address));
+	  MEM_VOLATILE_P (mem_mduc) = 1;       
+	  emit_insn (gen_movqi (gen_rtx_REG (QImode, A_REG), mem_mduc));
+	}
+	else
+	{
+	  mem_mduc = gen_rtx_MEM (HImode, GEN_INT (mduc_regs[i].address));
+	  MEM_VOLATILE_P (mem_mduc) = 1;       
+	  emit_insn (gen_movqi (gen_rtx_REG (HImode, AX_REG), mem_mduc));
+	}
+	emit_insn (gen_push (gen_rtx_REG (HImode, AX_REG)));
+      }
+    }
   if (frame_pointer_needed)
     {
       F (emit_move_insn (ax, sp));
@@ -1378,6 +1440,8 @@
     }
 
   fs = cfun->machine->framesize_locals + cfun->machine->framesize_outgoing;
+  if (MUST_SAVE_MDUC_REGISTERS && (!crtl->is_leaf || check_mduc_usage ()))
+    fs = fs + NUM_OF_MDUC_REGS * 2;
   if (fs > 0)
     {
       /* If we need to subtract more than 254*3 then it is faster and
@@ -1426,6 +1490,8 @@
   else
     {
       fs = cfun->machine->framesize_locals + cfun->machine->framesize_outgoing;
+      if (MUST_SAVE_MDUC_REGISTERS && (!crtl->is_leaf || check_mduc_usage ()))
+	fs = fs + NUM_OF_MDUC_REGS * 2;
       if (fs > 254 * 3)
 	{
 	  emit_move_insn (ax, sp);
@@ -1444,6 +1510,29 @@
 	}
     }
 
+  /* Restore MDUC registers from interrupt routine.  */
+  if (MUST_SAVE_MDUC_REGISTERS && (!crtl->is_leaf || check_mduc_usage ()))
+    {
+      rtx mem_mduc;
+
+      for (int i = NUM_OF_MDUC_REGS-1; i >= 0; i--)
+      {
+	emit_insn (gen_pop (gen_rtx_REG (HImode, AX_REG)));
+        if (mduc_regs[i].mode == QImode)
+        {
+          mem_mduc = gen_rtx_MEM (QImode, GEN_INT (mduc_regs[i].address));
+          MEM_VOLATILE_P (mem_mduc) = 1;
+	  emit_insn (gen_movqi (mem_mduc, gen_rtx_REG (QImode, A_REG)));
+        }
+        else
+        {
+          mem_mduc = gen_rtx_MEM (HImode, GEN_INT (mduc_regs[i].address));
+          MEM_VOLATILE_P (mem_mduc) = 1;
+	  emit_insn (gen_movhi (mem_mduc, gen_rtx_REG (HImode, AX_REG)));
+        }
+      }
+    }
+
   /* Save ES register inside interrupt functions.  */
   if (is_interrupt_func (cfun->decl) && cfun->machine->uses_es)
     {
@@ -1599,6 +1688,9 @@
 
   if (cfun->machine->uses_es)
     fprintf (file, "\t; uses ES register\n");
+
+  if (MUST_SAVE_MDUC_REGISTERS)
+    fprintf (file, "\t; preserves MDUC registers\n");
 }
 
 /* Return an RTL describing where a function return value of type RET_TYPE
Index: gcc/config/rl78/rl78.h
===================================================================
--- gcc/config/rl78/rl78.h	(revision 2871)
+++ gcc/config/rl78/rl78.h	(working copy)
@@ -28,6 +28,7 @@
 #define TARGET_G14	(rl78_cpu_type == CPU_G14)
 
 
+#define NUM_OF_MDUC_REGS 6
 
 #define TARGET_CPU_CPP_BUILTINS()		    \
   do                                                \
Index: gcc/config/rl78/rl78.md
===================================================================
--- gcc/config/rl78/rl78.md	(revision 2871)
+++ gcc/config/rl78/rl78.md	(working copy)
@@ -78,6 +78,7 @@
 (include "rl78-virt.md")
 (include "rl78-real.md")
 
+(define_attr "is_g13_muldiv_insn" "yes,no" (const_string "no"))
 
 ;; Function Prologue/Epilogue Instructions
 
@@ -416,7 +417,8 @@
 	movw    ax, 0xffff6     ; MDBL
 	movw    %h0, ax
         ; end of mulhi macro"
-  [(set_attr "valloc" "macax")]
+  [(set_attr "valloc" "macax")
+   (set_attr "is_g13_muldiv_insn" "yes")]
 )
 
 ;; 0xFFFF0 is MACR(L).  0xFFFF2 is MACR(H) but we don't care about it
@@ -496,7 +498,8 @@
 	movw	ax, !0xf00e0	; MDCL
 	movw	%H0, ax
 	; end of mulsi macro"
-  [(set_attr "valloc" "macax")]
+  [(set_attr "valloc" "macax")
+   (set_attr "is_g13_muldiv_insn" "yes")]
 )
 
 ;; start-sanitize-rl78
@@ -731,7 +734,8 @@
 	movw	%H3, ax		\n\
 	; end of udivmodsi macro";
       }
-  [(set_attr "valloc" "macax")]
+  [(set_attr "valloc" "macax")
+   (set_attr "is_g13_muldiv_insn" "yes")]
 )
 (define_insn "mov1"
   [(set (match_operand:QI         0 "rl78_nonimmediate_operand"   "=vU")
Index: gcc/config/rl78/rl78.opt
===================================================================
--- gcc/config/rl78/rl78.opt	(revision 2871)
+++ gcc/config/rl78/rl78.opt	(working copy)
@@ -103,4 +103,10 @@
 Target Mask(ES0)
 Assume ES is zero throughout program execution, use ES: for read-only data.
 
+msave-mduc-in-interrupts
+Target Mask(SAVE_MDUC_REGISTERS)
+Stores the MDUC registers in interrupt handlers for G13 target.
 
+mno-save-mduc-in-interrupts
+Target RejectNegative Mask(NO_SAVE_MDUC_REGISTERS)
+Does not save the MDUC registers in interrupt handlers for G13 target.
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 2871)
+++ gcc/doc/invoke.texi	(working copy)
@@ -18901,6 +18901,20 @@
 registers @code{r24..r31} are reserved for use in interrupt handlers.
 With this option enabled these registers can be used in ordinary
 functions as well.
+
+@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). The option will not have any effect if
+the target does not have multiply hardware, or if the interrupt
+function does not call any other function.
 @end table
 
 @node RS/6000 and PowerPC Options

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

[Nick Clifton]

Hi Kaushik,

> +/* Structure for G13 MDUC registers.  */
> +struct mduc_reg_type
> +{
> +  unsigned int address;
> +  enum machine_mode mode;
> +  bool is_volatile;
> +};
> +
> +struct mduc_reg_type  mduc_regs[NUM_OF_MDUC_REGS] =
> +  {{0xf00e8, QImode, true},
> +   {0xffff0, HImode, true},
> +   {0xffff2, HImode, true},
> +   {0xf2224, HImode, true},
> +   {0xf00e0, HImode, true},
> +   {0xf00e2, HImode, true}};

If the is_volatile field is true for all members of this array, why 
bother having it at all ?  (If I remember correctly in your previous 
patch only some of these addresses were being treated as volatile 
registers, not all of them).


> +/* Check if the block uses mul/div insns for G13 target.  */
> +static bool
> +check_mduc_usage ()

Add a void type to the declaration.  Ie:

   check mduc_usage (void)


> +{
> +  rtx insn;
> +  basic_block bb;
> +  FOR_EACH_BB_FN (bb, cfun)
> +  {

You should have a blank line between the end of the variable 
declarations and the start of the code.


> +    FOR_BB_INSNS (bb, insn)
> +    {
> +      if (get_attr_is_g13_muldiv_insn (insn) == IS_G13_MULDIV_INSN_YES)
> +        return true;

I am not sure - but it might be safer to check INSN_P(insn) first 
before checking for the muldiv attribute.


> +      for (int i = 0; i <NUM_OF_MDUC_REGS; i++)
> +      {
> +if (mduc_regs[i].mode == QImode)
> +{

Indentation.


> +  mem_mduc = gen_rtx_MEM (QImode, GEN_INT (mduc_regs[i].address));
> +  MEM_VOLATILE_P (mem_mduc) = 1;
> +  emit_insn (gen_movqi (gen_rtx_REG (QImode, A_REG), mem_mduc));
> +}
> +else
> +{
> +  mem_mduc = gen_rtx_MEM (HImode, GEN_INT (mduc_regs[i].address));
> +  MEM_VOLATILE_P (mem_mduc) = 1;
> +  emit_insn (gen_movqi (gen_rtx_REG (HImode, AX_REG), mem_mduc));
> +}

In the else case you are using gen_movqi to move an HImode value...

Also you could simplify the above code like this:

   for (int i = 0; i < NUM_OF_MDUC_REGS; i++)
     {
        mduc_reg_type *reg = mduc_regs + i;
        rtx mem_mduc = gen_rtx_MEM (reg->mode, GEN_INT (reg->address));

        MEM_VOLATILE_P (mem_mduc) = reg->is_volatile;
        if (reg->mode == QImode)
          emit_insn (gen_movqi (gen_rtx_REG (QImode, A_REG), mem_mduc));
        else
          emit_insn (gen_movhi (gen_rtx_REG (HImode, AX_REG), mem_mduc));
        emit_insn (gen_push (gen_rtx_REG (HImode, AX_REG)));
     }

>     fs = cfun->machine->framesize_locals + cfun->machine->framesize_outgoing;
> +  if (MUST_SAVE_MDUC_REGISTERS && (!crtl->is_leaf || check_mduc_usage ()))
> +    fs = fs + NUM_OF_MDUC_REGS * 2;
>     if (fs > 0)
>       {
>         /* If we need to subtract more than 254*3 then it is faster and
> @@ -1426,6 +1490,8 @@
>     else
>       {
>         fs = cfun->machine->framesize_locals + cfun->machine->framesize_outgoing;
> +      if (MUST_SAVE_MDUC_REGISTERS && (!crtl->is_leaf || check_mduc_usage ()))
> +fs = fs + NUM_OF_MDUC_REGS * 2;
>         if (fs > 254 * 3)

No - this is wrong.  "fs" is the amount of extra space needed in the 
stack frame to hold local variables and outgoing variables.  It should 
not include the stack space used for already pushed registers.


> Index: gcc/config/rl78/rl78.h
> ===================================================================
> --- gcc/config/rl78/rl78.h(revision 2871)
> +++ gcc/config/rl78/rl78.h(working copy)
> @@ -28,6 +28,7 @@
>   #define TARGET_G14(rl78_cpu_type == CPU_G14)
>
>
> +#define NUM_OF_MDUC_REGS 6

Why define this here ?  It is only ever used in rl78,c and it can be 
computed automatically by applying the ARRAY_SIZE macro to the mduc_regs 
array.



> Index: gcc/config/rl78/rl78.opt
> ===================================================================
> --- gcc/config/rl78/rl78.opt(revision 2871)
> +++ gcc/config/rl78/rl78.opt(working copy)
> @@ -103,4 +103,10 @@
>   Target Mask(ES0)
>   Assume ES is zero throughout program execution, use ES: for read-only data.
>
> +msave-mduc-in-interrupts
> +Target Mask(SAVE_MDUC_REGISTERS)
> +Stores the MDUC registers in interrupt handlers for G13 target.
>
> +mno-save-mduc-in-interrupts
> +Target RejectNegative Mask(NO_SAVE_MDUC_REGISTERS)
> +Does not save the MDUC registers in interrupt handlers for G13 target.

This looks wrong.  Surely you only need the msave-mduc-in-interrupts 
definition.  That will automatically allow -mno-save-mduc-in-interrupts, 
since it does not have the RejectNegative attribute.  Also these is no 
need to have two separate target mask bits.  Just SAVE_MDUC_REGISTERS 
will do.




> Index: gcc/doc/invoke.texi
> ===================================================================
> --- gcc/doc/invoke.texi(revision 2871)
> +++ gcc/doc/invoke.texi(working copy)

You should also add the name of the new option to the Machine Dependent 
Options section of the manual.  (Approx line 896 in invoke.texi)


> +@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). The option will not have any effect if
> +the target does not have multiply hardware, or if the interrupt
> +function does not call any other function.

Still not quite right.  The last sentence should be:

   The MDUC registers will only be saved if the interrupt handler 
performs a multiplication or division operation or it calls another 
function.

Cheers
   Nick

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

[Mike Stump]

On Jan 11, 2016, at 11:20 PM, Kaushik M Phatak <kphatak@gmail.com> wrote:
> Kindly review the updated patch and let me know if it is OK.

My review comment is still outstanding.

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

[Kaushik Phatak]

[-- Attachment #1: Type: text/plain, Size: 12300 bytes --]


Hi Nick,
I have modified and updated this patch as per your comments.
Apologies, as it has taken me awhile for me to get back to this.
https://gcc.gnu.org/ml/gcc-patches/2016-01/msg00702.html

>> +/* Structure for G13 MDUC registers.  */ struct mduc_reg_type {
>> +  unsigned int address;
>> +  enum machine_mode mode;
>> +  bool is_volatile;

>> +struct mduc_reg_type  mduc_regs[NUM_OF_MDUC_REGS] =
>> +  {{0xf00e8, QImode, true},

> If the is_volatile field is true for all members of this array, why bother having it at all ?

I have got rid of the unnecessary volatile field here.

>> +check_mduc_usage ()
>Add a void type to the declaration.

Done.

> You should have a blank line between the end of the variable 
> declarations and the start of the code.

Done.

>> > +      if (get_attr_is_g13_muldiv_insn (insn) == IS_G13_MULDIV_INSN_YES)
> I am not sure - but it might be safer to check INSN_P(insn) first

Added an INSN_P check here.

>> > +if (mduc_regs[i].mode == QImode)
>> +{
> Indentation.

Hopefully this is fixed. Some issue in editor when moving from tabs to spaces.

>> +  emit_insn (gen_movqi (gen_rtx_REG (HImode, AX_REG), mem_mduc));
>> +}
>In the else case you are using gen_movqi to move an HImode value...
>Also you could simplify the above code like this:

Fixed this, also used a simpler logic as suggested.

>> > +fs = fs + NUM_OF_MDUC_REGS * 2;
>>         if (fs > 254 * 3)
> No - this is wrong.  "fs" is the amount of extra space needed in the

Sorry, I think I misread this. I have added code at top or prologue which
will update cfun->machine->framesize.

>> +#define NUM_OF_MDUC_REGS 6
> Why define this here ?  It is only ever used in rl78,c and it can be 
> computed automatically by applying the ARRAY_SIZE macro

I have removed this marco and used ARRAY_SIZE to compute the size instead.

>> +msave-mduc-in-interrupts
>> +Target Mask(SAVE_MDUC_REGISTERS)
>> +Stores the MDUC registers in interrupt handlers for G13 target.
>>
>> +mno-save-mduc-in-interrupts
>> +Target RejectNegative Mask(NO_SAVE_MDUC_REGISTERS)
>> +Does not save the MDUC registers in interrupt handlers for G13 target.

>This looks wrong.  Surely you only need the msave-mduc-in-interrupts
>definition.  That will automatically allow -mno-save-mduc-in-interrupts, 
>since it does not have the RejectNegative attribute.  Also these is no 
>need to have two separate target mask bits.  Just SAVE_MDUC_REGISTERS 
>will do.

Well, the earlier idea was to save the MDUC registers by default for G13 targets.
Hence the '-mno-' was introduced, but I can go with your suggestion if it reduces
any confusion.

>> +++ gcc/doc/invoke.texi(working copy)
> You should also add the name of the new option to the Machine Dependent

Added the new option to the list.

>> +@item -msave-mduc-in-interrupts
>Still not quite right.  The last sentence should be:
>  The MDUC registers will only be saved

Update the last line of the manual as per your suggestion.

>> My review comment is still outstanding. - from Mike Stump

The current RL78 ABI does not contain specific information about these registers
from the G13 variant of the RL78 target. We can try and request Renesas to add information
about the same along with the option required for this.
Nick, do you have any thoughts on this? (assuming this version of patch is closer to acceptance)

The patch is regression tested for "-msim -mg13 -msave-mduc-in-interrupts".

Best Regards,
Kaushik 

gcc/ChangeLog
2016-05-04  Kaushik Phatak  <kaushik.phatak@kpit.com>

	* config/rl78/rl78.c (rl78_expand_prologue): Save the MDUC related
	registers in all interrupt handlers if necessary.
	(rl78_option_override): Add warning.
	(MUST_SAVE_MDUC_REGISTERS): New macro.
	(rl78_expand_epilogue): Restore the MDUC registers if necessary.
	* config/rl78/rl78.c (check_mduc_usage): New function.
	* config/rl78/rl78.c (mduc_regs): New structure to hold MDUC register data.
	* config/rl78/rl78.md (is_g13_muldiv_insn): New attribute.
	* config/rl78/rl78.md (mulsi3_g13): Add is_g13_muldiv_insn attribute.
	* config/rl78/rl78.md (udivmodsi4_g13): Add is_g13_muldiv_insn attribute.
	* config/rl78/rl78.md (mulhi3_g13): Add is_g13_muldiv_insn attribute.
	* config/rl78/rl78.opt (msave-mduc-in-interrupts): New option.
	* doc/invoke.texi (RL78 Options): Add -msave-mduc-in-interrupts.


Index: gcc/config/rl78/rl78.c
===================================================================
--- gcc/config/rl78/rl78.c	(revision 235865)
+++ gcc/config/rl78/rl78.c	(working copy)
@@ -76,6 +76,21 @@
   "sp", "ap", "psw", "es", "cs"
 };
 
+/* Structure for G13 MDUC registers.  */
+struct mduc_reg_type
+{
+  unsigned int address;
+  enum machine_mode mode;
+};
+
+struct mduc_reg_type  mduc_regs[] =
+  {{0xf00e8, QImode},
+   {0xffff0, HImode},
+   {0xffff2, HImode},
+   {0xf2224, HImode},
+   {0xf00e0, HImode},
+   {0xf00e2, HImode}};
+
 struct GTY(()) machine_function
 {
   /* If set, the rest of the fields have been computed.  */
@@ -317,6 +332,10 @@
 #undef  TARGET_OPTION_OVERRIDE
 #define TARGET_OPTION_OVERRIDE		rl78_option_override
 
+#define MUST_SAVE_MDUC_REGISTERS			\
+  (TARGET_SAVE_MDUC_REGISTERS				\
+   && (is_interrupt_func (NULL_TREE)) && RL78_MUL_G13)
+
 static void
 rl78_option_override (void)
 {
@@ -344,6 +363,9 @@
     /* Address spaces are currently only supported by C.  */
     error ("-mes0 can only be used with C");
 
+  if (TARGET_SAVE_MDUC_REGISTERS && !(TARGET_G13 || RL78_MUL_G13))
+    warning (0, "mduc registers only saved for G13 target");
+
   switch (rl78_cpu_type)
     {
     case CPU_UNINIT:
@@ -1263,6 +1285,25 @@
   return (lookup_attribute ("naked", DECL_ATTRIBUTES (current_function_decl)) != NULL_TREE);
 }
 
+/* Check if the block uses mul/div insns for G13 target.  */
+static bool
+check_mduc_usage (void)
+{
+  rtx insn;
+  basic_block bb;
+
+  FOR_EACH_BB_FN (bb, cfun)
+    {
+      FOR_BB_INSNS (bb, insn)
+        {
+          if (INSN_P (insn)
+              && (get_attr_is_g13_muldiv_insn (insn) == IS_G13_MULDIV_INSN_YES))
+          return true;
+    }
+  }
+  return false;
+}
+
 /* Expand the function prologue (from the prologue pattern).  */
 void
 rl78_expand_prologue (void)
@@ -1278,6 +1319,9 @@
   /* Always re-compute the frame info - the register usage may have changed.  */
   rl78_compute_frame_info ();
 
+  if (MUST_SAVE_MDUC_REGISTERS && (!crtl->is_leaf || check_mduc_usage ()))
+    cfun->machine->framesize += ARRAY_SIZE (mduc_regs) * 2;
+
   if (flag_stack_usage_info)
     current_function_static_stack_size = cfun->machine->framesize;
 
@@ -1327,6 +1371,24 @@
       F (emit_insn (gen_push (ax)));
     }
 
+  /* Save MDUC registers inside interrupt routine.  */
+  if (MUST_SAVE_MDUC_REGISTERS && (!crtl->is_leaf || check_mduc_usage ()))
+    {
+      for (int i = 0; i < ARRAY_SIZE (mduc_regs); i++)
+        {
+          mduc_reg_type *reg = mduc_regs + i;
+          rtx mem_mduc = gen_rtx_MEM (reg->mode, GEN_INT (reg->address));
+
+          MEM_VOLATILE_P (mem_mduc) = 1;
+          if (reg->mode == QImode)
+            emit_insn (gen_movqi (gen_rtx_REG (QImode, A_REG), mem_mduc));
+          else
+            emit_insn (gen_movhi (gen_rtx_REG (HImode, AX_REG), mem_mduc));
+
+          emit_insn (gen_push (gen_rtx_REG (HImode, AX_REG)));
+        }
+    }
+
   if (frame_pointer_needed)
     {
       F (emit_move_insn (ax, sp));
@@ -1400,6 +1462,23 @@
 	}
     }
 
+  /* Restore MDUC registers from interrupt routine.  */
+  if (MUST_SAVE_MDUC_REGISTERS && (!crtl->is_leaf || check_mduc_usage ()))
+    {
+      for (int i = ARRAY_SIZE (mduc_regs) - 1; i >= 0; i--)
+        {
+          mduc_reg_type *reg = mduc_regs + i;
+          rtx mem_mduc = gen_rtx_MEM (reg->mode, GEN_INT (reg->address));
+
+          emit_insn (gen_pop (gen_rtx_REG (HImode, AX_REG)));
+          MEM_VOLATILE_P (mem_mduc) = 1;
+          if (reg->mode == QImode)
+            emit_insn (gen_movqi (mem_mduc, gen_rtx_REG (QImode, A_REG)));
+          else
+            emit_insn (gen_movhi (mem_mduc, gen_rtx_REG (HImode, AX_REG)));
+        }
+    }
+
   if (is_interrupt_func (cfun->decl) && cfun->machine->uses_es)
     {
       emit_insn (gen_pop (gen_rtx_REG (HImode, AX_REG)));
@@ -1495,6 +1574,9 @@
 
   if (cfun->machine->uses_es)
     fprintf (file, "\t; uses ES register\n");
+
+  if (MUST_SAVE_MDUC_REGISTERS)
+    fprintf (file, "\t; preserves MDUC registers\n");
 }
 
 /* Return an RTL describing where a function return value of type RET_TYPE
Index: gcc/config/rl78/rl78.md
===================================================================
--- gcc/config/rl78/rl78.md	(revision 235865)
+++ gcc/config/rl78/rl78.md	(working copy)
@@ -67,6 +67,7 @@
 (include "rl78-virt.md")
 (include "rl78-real.md")
 
+(define_attr "is_g13_muldiv_insn" "yes,no" (const_string "no"))
 
 ;; Function Prologue/Epilogue Instructions
 
@@ -379,7 +380,8 @@
 	movw    ax, 0xffff6     ; MDBL
 	movw    %h0, ax
         ; end of mulhi macro"
-  [(set_attr "valloc" "macax")]
+  [(set_attr "valloc" "macax")
+   (set_attr "is_g13_muldiv_insn" "yes")]
 )
 
 ;; 0xFFFF0 is MACR(L).  0xFFFF2 is MACR(H) but we don't care about it
@@ -459,7 +461,8 @@
 	movw	ax, !0xf00e0	; MDCL
 	movw	%H0, ax
 	; end of mulsi macro"
-  [(set_attr "valloc" "macax")]
+  [(set_attr "valloc" "macax")
+   (set_attr "is_g13_muldiv_insn" "yes")]
 )
 
 (define_expand "udivmodhi4"
@@ -692,5 +695,6 @@
 	movw	%H3, ax		\n\
 	; end of udivmodsi macro";
       }
-  [(set_attr "valloc" "macax")]
+  [(set_attr "valloc" "macax")
+   (set_attr "is_g13_muldiv_insn" "yes")]
 )
Index: gcc/config/rl78/rl78.opt
===================================================================
--- gcc/config/rl78/rl78.opt	(revision 235865)
+++ gcc/config/rl78/rl78.opt	(working copy)
@@ -91,3 +91,7 @@
 mes0
 Target Mask(ES0)
 Assume ES is zero throughout program execution, use ES: for read-only data.
+
+msave-mduc-in-interrupts
+Target Mask(SAVE_MDUC_REGISTERS)
+Stores the MDUC registers in interrupt handlers for G13 target.
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 235865)
+++ gcc/doc/invoke.texi	(working copy)
@@ -946,7 +946,7 @@
 @emph{RL78 Options}
 @gccoptlist{-msim -mmul=none -mmul=g13 -mmul=g14 -mallregs @gol
 -mcpu=g10 -mcpu=g13 -mcpu=g14 -mg10 -mg13 -mg14 @gol
--m64bit-doubles -m32bit-doubles}
+-m64bit-doubles -m32bit-doubles -msave-mduc-in-interrupts}
 
 @emph{RS/6000 and PowerPC Options}
 @gccoptlist{-mcpu=@var{cpu-type} @gol
@@ -19777,6 +19777,20 @@
 or 32 bits (@option{-m32bit-doubles}) in size.  The default is
 @option{-m32bit-doubles}.
 
+@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). The MDUC registers will only be saved
+if the interrupt handler performs a multiplication or division
+operation or it calls another function.
+
 @end table
 
 @node RS/6000 and PowerPC Options


===== END of PATCH ======


-----Original Message-----
From: Mike Stump [mailto:mikestump@comcast.net] 
Sent: Tuesday, January 12, 2016 9:22 PM
To: Kaushik M Phatak <kphatak@gmail.com>
Cc: Nick Clifton <nickc@redhat.com>; dj@redhat.com; gcc-patches@gcc.gnu.org; Kaushik Phatak <Kaushik.Phatak@kpit.com>
Subject: Re: [PATCH : RL78] Disable interrupts during hardware multiplication routines

On Jan 11, 2016, at 11:20 PM, Kaushik M Phatak <kphatak@gmail.com> wrote:
> Kindly review the updated patch and let me know if it is OK.

My review comment is still outstanding.

[-- Attachment #2: rl78_mduc_patch.diff --]
[-- Type: application/octet-stream, Size: 7053 bytes --]

Index: gcc/config/rl78/rl78.c
===================================================================
--- gcc/config/rl78/rl78.c	(revision 235865)
+++ gcc/config/rl78/rl78.c	(working copy)
@@ -76,6 +76,21 @@
   "sp", "ap", "psw", "es", "cs"
 };
 
+/* Structure for G13 MDUC registers.  */
+struct mduc_reg_type
+{
+  unsigned int address;
+  enum machine_mode mode;
+};
+
+struct mduc_reg_type  mduc_regs[] =
+  {{0xf00e8, QImode},
+   {0xffff0, HImode},
+   {0xffff2, HImode},
+   {0xf2224, HImode},
+   {0xf00e0, HImode},
+   {0xf00e2, HImode}};
+
 struct GTY(()) machine_function
 {
   /* If set, the rest of the fields have been computed.  */
@@ -317,6 +332,10 @@
 #undef  TARGET_OPTION_OVERRIDE
 #define TARGET_OPTION_OVERRIDE		rl78_option_override
 
+#define MUST_SAVE_MDUC_REGISTERS			\
+  (TARGET_SAVE_MDUC_REGISTERS				\
+   && (is_interrupt_func (NULL_TREE)) && RL78_MUL_G13)
+
 static void
 rl78_option_override (void)
 {
@@ -344,6 +363,9 @@
     /* Address spaces are currently only supported by C.  */
     error ("-mes0 can only be used with C");
 
+  if (TARGET_SAVE_MDUC_REGISTERS && !(TARGET_G13 || RL78_MUL_G13))
+    warning (0, "mduc registers only saved for G13 target");
+
   switch (rl78_cpu_type)
     {
     case CPU_UNINIT:
@@ -1263,6 +1285,25 @@
   return (lookup_attribute ("naked", DECL_ATTRIBUTES (current_function_decl)) != NULL_TREE);
 }
 
+/* Check if the block uses mul/div insns for G13 target.  */
+static bool
+check_mduc_usage (void)
+{
+  rtx insn;
+  basic_block bb;
+
+  FOR_EACH_BB_FN (bb, cfun)
+    {
+      FOR_BB_INSNS (bb, insn)
+        {
+          if (INSN_P (insn)
+              && (get_attr_is_g13_muldiv_insn (insn) == IS_G13_MULDIV_INSN_YES))
+          return true;
+    }
+  }
+  return false;
+}
+
 /* Expand the function prologue (from the prologue pattern).  */
 void
 rl78_expand_prologue (void)
@@ -1278,6 +1319,9 @@
   /* Always re-compute the frame info - the register usage may have changed.  */
   rl78_compute_frame_info ();
 
+  if (MUST_SAVE_MDUC_REGISTERS && (!crtl->is_leaf || check_mduc_usage ()))
+    cfun->machine->framesize += ARRAY_SIZE (mduc_regs) * 2;
+
   if (flag_stack_usage_info)
     current_function_static_stack_size = cfun->machine->framesize;
 
@@ -1327,6 +1371,24 @@
       F (emit_insn (gen_push (ax)));
     }
 
+  /* Save MDUC registers inside interrupt routine.  */
+  if (MUST_SAVE_MDUC_REGISTERS && (!crtl->is_leaf || check_mduc_usage ()))
+    {
+      for (int i = 0; i < ARRAY_SIZE (mduc_regs); i++)
+        {
+          mduc_reg_type *reg = mduc_regs + i;
+          rtx mem_mduc = gen_rtx_MEM (reg->mode, GEN_INT (reg->address));
+
+          MEM_VOLATILE_P (mem_mduc) = 1;
+          if (reg->mode == QImode)
+            emit_insn (gen_movqi (gen_rtx_REG (QImode, A_REG), mem_mduc));
+          else
+            emit_insn (gen_movhi (gen_rtx_REG (HImode, AX_REG), mem_mduc));
+
+          emit_insn (gen_push (gen_rtx_REG (HImode, AX_REG)));
+        }
+    }
+
   if (frame_pointer_needed)
     {
       F (emit_move_insn (ax, sp));
@@ -1400,6 +1462,23 @@
 	}
     }
 
+  /* Restore MDUC registers from interrupt routine.  */
+  if (MUST_SAVE_MDUC_REGISTERS && (!crtl->is_leaf || check_mduc_usage ()))
+    {
+      for (int i = ARRAY_SIZE (mduc_regs) - 1; i >= 0; i--)
+        {
+          mduc_reg_type *reg = mduc_regs + i;
+          rtx mem_mduc = gen_rtx_MEM (reg->mode, GEN_INT (reg->address));
+
+          emit_insn (gen_pop (gen_rtx_REG (HImode, AX_REG)));
+          MEM_VOLATILE_P (mem_mduc) = 1;
+          if (reg->mode == QImode)
+            emit_insn (gen_movqi (mem_mduc, gen_rtx_REG (QImode, A_REG)));
+          else
+            emit_insn (gen_movhi (mem_mduc, gen_rtx_REG (HImode, AX_REG)));
+        }
+    }
+
   if (is_interrupt_func (cfun->decl) && cfun->machine->uses_es)
     {
       emit_insn (gen_pop (gen_rtx_REG (HImode, AX_REG)));
@@ -1495,6 +1574,9 @@
 
   if (cfun->machine->uses_es)
     fprintf (file, "\t; uses ES register\n");
+
+  if (MUST_SAVE_MDUC_REGISTERS)
+    fprintf (file, "\t; preserves MDUC registers\n");
 }
 
 /* Return an RTL describing where a function return value of type RET_TYPE
Index: gcc/config/rl78/rl78.md
===================================================================
--- gcc/config/rl78/rl78.md	(revision 235865)
+++ gcc/config/rl78/rl78.md	(working copy)
@@ -67,6 +67,7 @@
 (include "rl78-virt.md")
 (include "rl78-real.md")
 
+(define_attr "is_g13_muldiv_insn" "yes,no" (const_string "no"))
 
 ;; Function Prologue/Epilogue Instructions
 
@@ -379,7 +380,8 @@
 	movw    ax, 0xffff6     ; MDBL
 	movw    %h0, ax
         ; end of mulhi macro"
-  [(set_attr "valloc" "macax")]
+  [(set_attr "valloc" "macax")
+   (set_attr "is_g13_muldiv_insn" "yes")]
 )
 
 ;; 0xFFFF0 is MACR(L).  0xFFFF2 is MACR(H) but we don't care about it
@@ -459,7 +461,8 @@
 	movw	ax, !0xf00e0	; MDCL
 	movw	%H0, ax
 	; end of mulsi macro"
-  [(set_attr "valloc" "macax")]
+  [(set_attr "valloc" "macax")
+   (set_attr "is_g13_muldiv_insn" "yes")]
 )
 
 (define_expand "udivmodhi4"
@@ -692,5 +695,6 @@
 	movw	%H3, ax		\n\
 	; end of udivmodsi macro";
       }
-  [(set_attr "valloc" "macax")]
+  [(set_attr "valloc" "macax")
+   (set_attr "is_g13_muldiv_insn" "yes")]
 )
Index: gcc/config/rl78/rl78.opt
===================================================================
--- gcc/config/rl78/rl78.opt	(revision 235865)
+++ gcc/config/rl78/rl78.opt	(working copy)
@@ -91,3 +91,7 @@
 mes0
 Target Mask(ES0)
 Assume ES is zero throughout program execution, use ES: for read-only data.
+
+msave-mduc-in-interrupts
+Target Mask(SAVE_MDUC_REGISTERS)
+Stores the MDUC registers in interrupt handlers for G13 target.
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 235865)
+++ gcc/doc/invoke.texi	(working copy)
@@ -946,7 +946,7 @@
 @emph{RL78 Options}
 @gccoptlist{-msim -mmul=none -mmul=g13 -mmul=g14 -mallregs @gol
 -mcpu=g10 -mcpu=g13 -mcpu=g14 -mg10 -mg13 -mg14 @gol
--m64bit-doubles -m32bit-doubles}
+-m64bit-doubles -m32bit-doubles -msave-mduc-in-interrupts}
 
 @emph{RS/6000 and PowerPC Options}
 @gccoptlist{-mcpu=@var{cpu-type} @gol
@@ -19777,6 +19777,20 @@
 or 32 bits (@option{-m32bit-doubles}) in size.  The default is
 @option{-m32bit-doubles}.
 
+@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). The MDUC registers will only be saved
+if the interrupt handler performs a multiplication or division
+operation or it calls another function.
+
 @end table
 
 @node RS/6000 and PowerPC Options

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

[Nick Clifton]

Hi Kaushik,

> gcc/ChangeLog
> 2016-05-04  Kaushik Phatak  <kaushik.phatak@kpit.com>
> 
> 	* config/rl78/rl78.c (rl78_expand_prologue): Save the MDUC related
> 	registers in all interrupt handlers if necessary.
> 	(rl78_option_override): Add warning.
> 	(MUST_SAVE_MDUC_REGISTERS): New macro.
> 	(rl78_expand_epilogue): Restore the MDUC registers if necessary.
> 	* config/rl78/rl78.c (check_mduc_usage): New function.
> 	* config/rl78/rl78.c (mduc_regs): New structure to hold MDUC register data.
> 	* config/rl78/rl78.md (is_g13_muldiv_insn): New attribute.
> 	* config/rl78/rl78.md (mulsi3_g13): Add is_g13_muldiv_insn attribute.
> 	* config/rl78/rl78.md (udivmodsi4_g13): Add is_g13_muldiv_insn attribute.
> 	* config/rl78/rl78.md (mulhi3_g13): Add is_g13_muldiv_insn attribute.
> 	* config/rl78/rl78.opt (msave-mduc-in-interrupts): New option.
> 	* doc/invoke.texi (RL78 Options): Add -msave-mduc-in-interrupts.
 
Approved and applied - thanks for persevering with this.

Cheers
  Nick

[PATCH : RL78] Disable interrupts during hardware multiplication routines

[Kaushik Phatak]

[-- Attachment #1: Type: text/plain, Size: 3227 bytes --]

Hi,
Please find attached a patch which disables interrupts during inline hardware multiplication routines.
These routines use up several control registers which are not saved/restored in interrupt routines.
This causes corruption of result in case multiplication/division registers are used in main code as well as interrupts.

This patch has been regression tested with simulator as well as hardware.
Please review the same and let me know if OK to commit?

Best Regards,
Kaushik Phatak

2015-06-05  Kaushik Phatak  <kaushik.phatak@kpit.com>
        * config/rl78/rl78.md (mulhi3_g13): Disable interrupts in
		routine.
        (mulsi3_g14): Likewise.
        (mulsi3_g13): Likewise.
		(udivmodsi4_g13): Likewise.
		
Index: gcc/config/rl78/rl78.md
===================================================================
--- gcc/config/rl78/rl78.md	(revision 224145)
+++ gcc/config/rl78/rl78.md	(working copy)
@@ -372,6 +372,7 @@
   ]
   "RL78_MUL_G13"
   "; G13 mulhi macro %0 = %1 * %2
+	di
 	mov     a, #0x00
 	mov     !0xf00e8, a     ; MDUC
 	movw    ax, %h1
@@ -381,6 +382,7 @@
 	nop     ; mdb = mdal * mdah
 	movw    ax, 0xffff6     ; MDBL
 	movw    %h0, ax
+	ei
         ; end of mulhi macro"
   [(set_attr "valloc" "macax")]
 )
@@ -397,6 +399,7 @@
   ]
   "RL78_MUL_G14"
   "; G14 mulsi macro %0 = %1 * %2
+	di
 	movw	ax, %h1
 	movw	bc, %h2
 	MULHU	; bcax = bc * ax
@@ -411,6 +414,7 @@
 	MACHU	; MACR += bc * ax
 	movw	ax, 0xffff0
 	movw	%H0, ax
+	ei
 	; end of mulsi macro"
   [(set_attr "valloc" "macax")]
   )
@@ -429,6 +433,7 @@
   ]
   "RL78_MUL_G13"
   "; G13 mulsi macro %0 = %1 * %2
+	di
 	mov	a, #0x00
 	mov	!0xf00e8, a	; MDUC
 	movw	ax, %h1
@@ -461,6 +466,7 @@
 	nop	; Additional nop for MAC
 	movw	ax, !0xf00e0	; MDCL
 	movw	%H0, ax
+	ei
 	; end of mulsi macro"
   [(set_attr "valloc" "macax")]
 )
@@ -629,6 +635,7 @@
   {
     if (find_reg_note (insn, REG_UNUSED, operands[3]))
       return "; G13 udivsi macro %0 = %1 / %2 \n\
+	di	\n\
 	mov	a, #0xC0	; Set DIVMODE=1 and MACMODE=1 \n\
 	mov	!0xf00e8, a	; This preps the peripheral for division without interrupt generation \n\
 	movw	ax, %H1		\n\
@@ -647,9 +654,11 @@
 	movw	%h0, ax		\n\
 	movw    ax, 0xffff2	\n\
 	movw	%H0, ax		\n\
+	ei	\n\
 	; end of udivsi macro";
     else if (find_reg_note (insn, REG_UNUSED, operands[0]))
       return "; G13 umodsi macro %3 = %1 %% %2 \n\
+	di	\n\
 	mov	a, #0xC0	; Set DIVMODE=1 and MACMODE=1 \n\
 	mov	!0xf00e8, a	; This preps the peripheral for division without interrupt generation \n\
 	movw	ax, %H1		\n\
@@ -668,9 +677,11 @@
 	movw	%h3, ax		\n\
 	movw	ax, !0xf00e2	\n\
 	movw	%H3, ax		\n\
+	ei	\n\
 	; end of umodsi macro";
     else
       return "; G13 udivmodsi macro %0 = %1 / %2 and %3 = %1 %% %2 \n\
+	di	\n\
 	mov	a, #0xC0	; Set DIVMODE=1 and MACMODE=1 \n\
 	mov	!0xf00e8, a	; This preps the peripheral for division without interrupt generation \n\
 	movw	ax, %H1		\n\
@@ -693,6 +704,7 @@
 	movw	%h3, ax		\n\
 	movw	ax, !0xf00e2	\n\
 	movw	%H3, ax		\n\
+	ei	\n\
 	; end of udivmodsi macro";
       }
   [(set_attr "valloc" "macax")]
		
/* End of Patch  */

Best Regards,
Kaushik Phatak



[-- Attachment #2: rl78_mul_div.diff --]
[-- Type: application/octet-stream, Size: 2338 bytes --]

Index: gcc/config/rl78/rl78.md
===================================================================
--- gcc/config/rl78/rl78.md	(revision 224145)
+++ gcc/config/rl78/rl78.md	(working copy)
@@ -372,6 +372,7 @@
   ]
   "RL78_MUL_G13"
   "; G13 mulhi macro %0 = %1 * %2
+	di
 	mov     a, #0x00
 	mov     !0xf00e8, a     ; MDUC
 	movw    ax, %h1
@@ -381,6 +382,7 @@
 	nop     ; mdb = mdal * mdah
 	movw    ax, 0xffff6     ; MDBL
 	movw    %h0, ax
+	ei
         ; end of mulhi macro"
   [(set_attr "valloc" "macax")]
 )
@@ -397,6 +399,7 @@
   ]
   "RL78_MUL_G14"
   "; G14 mulsi macro %0 = %1 * %2
+	di
 	movw	ax, %h1
 	movw	bc, %h2
 	MULHU	; bcax = bc * ax
@@ -411,6 +414,7 @@
 	MACHU	; MACR += bc * ax
 	movw	ax, 0xffff0
 	movw	%H0, ax
+	ei
 	; end of mulsi macro"
   [(set_attr "valloc" "macax")]
   )
@@ -429,6 +433,7 @@
   ]
   "RL78_MUL_G13"
   "; G13 mulsi macro %0 = %1 * %2
+	di
 	mov	a, #0x00
 	mov	!0xf00e8, a	; MDUC
 	movw	ax, %h1
@@ -461,6 +466,7 @@
 	nop	; Additional nop for MAC
 	movw	ax, !0xf00e0	; MDCL
 	movw	%H0, ax
+	ei
 	; end of mulsi macro"
   [(set_attr "valloc" "macax")]
 )
@@ -629,6 +635,7 @@
   {
     if (find_reg_note (insn, REG_UNUSED, operands[3]))
       return "; G13 udivsi macro %0 = %1 / %2 \n\
+	di	\n\
 	mov	a, #0xC0	; Set DIVMODE=1 and MACMODE=1 \n\
 	mov	!0xf00e8, a	; This preps the peripheral for division without interrupt generation \n\
 	movw	ax, %H1		\n\
@@ -647,9 +654,11 @@
 	movw	%h0, ax		\n\
 	movw    ax, 0xffff2	\n\
 	movw	%H0, ax		\n\
+	ei	\n\
 	; end of udivsi macro";
     else if (find_reg_note (insn, REG_UNUSED, operands[0]))
       return "; G13 umodsi macro %3 = %1 %% %2 \n\
+	di	\n\
 	mov	a, #0xC0	; Set DIVMODE=1 and MACMODE=1 \n\
 	mov	!0xf00e8, a	; This preps the peripheral for division without interrupt generation \n\
 	movw	ax, %H1		\n\
@@ -668,9 +677,11 @@
 	movw	%h3, ax		\n\
 	movw	ax, !0xf00e2	\n\
 	movw	%H3, ax		\n\
+	ei	\n\
 	; end of umodsi macro";
     else
       return "; G13 udivmodsi macro %0 = %1 / %2 and %3 = %1 %% %2 \n\
+	di	\n\
 	mov	a, #0xC0	; Set DIVMODE=1 and MACMODE=1 \n\
 	mov	!0xf00e8, a	; This preps the peripheral for division without interrupt generation \n\
 	movw	ax, %H1		\n\
@@ -693,6 +704,7 @@
 	movw	%h3, ax		\n\
 	movw	ax, !0xf00e2	\n\
 	movw	%H3, ax		\n\
+	ei	\n\
 	; end of udivmodsi macro";
       }
   [(set_attr "valloc" "macax")]

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

[DJ Delorie]

Have you compared the latency of the multiply instructions to the
overhead of saving those registers in the interrupt handler?  What
about the case where performance is priority, and the developer knows
that the interrupt handlers don't use the multiply registers?

Also, your code doesn't properly handle the case where the interrupts
are already disabled when those functions are called.  It would
re-enable interrupts before the main code was prepared for it.

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

[Kaushik Phatak]

Hi DJ,
Thanks for your feedback comments. Sorry it took me a while to get back on this.

>> Have you compared the latency of the multiply instructions to the overhead of
>> saving those registers in the interrupt handler?
>> What about the case where performance is priority, and the developer knows that
>> the interrupt handlers don't use the multiply registers?
>> Also, your code doesn't properly handle the case where the interrupts are alread
>> disabled when those functions are called.  It would re-enable interrupts before 
>> the main code was prepared for it.

Yes, I agree the patch does not handle the case where interrupts are disabled. 
Also, the code performance would suffer when the 'di/ei' instructions are placed 
inline with the multiplication code.

I have worked out an updated patch, which would save the MDUC specific registers
in the interrupt routine when the option '-msave-mduc-in-interrupts' is passed.
This gets active only for the G13 targets.
This patch will save and restore the MDUC specific registers: mduc,mdal/h,mdbl/h and mdcl/h
This option does add about 56 bytes of code to the interrupt service routine due
to the saves/restores via push/pop.
Kindly review this patch and let me know if it would be useful for the RL78 port 
(either in current state or with modifications)
The other option/solution would be for the end user to disable/enable interrupts during the
mul/div routines in project as per their requirement.

This has been regression tested for ""-mg13 -msim -msave-mduc-in-interrupts"

Best Regards,
Kaushik

gcc/ChangeLog
2015-08-27  Kaushik Phatak  <kaushik.phatak@kpit.com>

	* config/rl78/rl78-real.md (movqi_from_mduc,movhi_from_mdal,
	movhi_from_mdah,movhi_from_mdbl,movhi_from_mdbh,movhi_from_mdcl,
	movhi_from_mdch,movqi_to_mduc,movhi_to_mdal,movhi_to_mdah,
	movhi_to_mdbl,movhi_to_mdbh,movhi_to_mdcl,movhi_to_mdch): New patterns.
	* config/rl78/rl78.c (rl78_expand_prologue): Save the MDUC related
	register in all interrupt handlers if necessary.
	(rl78_option_override): Add warning.
	(MUST_SAVE_MDUC_REGISTER): New macro.
	(rl78_expand_epilogue): Restore the MDUC registers if necessary.
	* config/rl78/rl78.opt (msave-mduc-in-interrupts): New option.
	* doc/invoke.texi (@item -msave-mduc-in-interrupts): New item.
	
Index: gcc/config/rl78/rl78-real.md
===================================================================
--- gcc/config/rl78/rl78-real.md	(revision 227024)
+++ gcc/config/rl78/rl78-real.md	(working copy)
@@ -37,6 +37,55 @@
 
 ;;---------- Moving ------------------------
 
+(define_insn "movqi_from_mduc"
+  [(set (match_operand:QI 0 "register_operand" "=a")
+	(unspec_volatile:QI [(match_operand:QI 1 "" "")] UNS_BUILTIN_MDUC))]
+  ""
+  "mov\t%0, !0xf00e8"
+)
+
+(define_insn "movhi_from_mdal"
+  [(set (match_operand:HI 0 "register_operand" "=A")
+	(unspec_volatile:HI [(match_operand:HI 1 "" "")] UNS_BUILTIN_MDAL))]
+  ""
+  "movw\t%0, !0xffff0"
+)
+
+(define_insn "movhi_from_mdah"
+  [(set (match_operand:HI 0 "register_operand" "=A")
+	(unspec_volatile:HI [(match_operand:HI 1 "" "")] UNS_BUILTIN_MDAH))]
+  ""
+  "movw\t%0, !0xffff2"
+)
+
+(define_insn "movhi_from_mdbl"
+  [(set (match_operand:HI 0 "register_operand" "=A")
+	(unspec_volatile:HI [(match_operand 1)] UNS_BUILTIN_MDBL))]
+  ""
+  "movw\t%0, !0xffff4"
+)
+
+(define_insn "movhi_from_mdbh"
+  [(set (match_operand:HI 0 "register_operand" "=A")
+	(unspec_volatile:HI [(match_operand 1)] UNS_BUILTIN_MDBH))]
+  ""
+  "movw\t%0, !0xffff6"
+)
+
+(define_insn "movhi_from_mdcl"
+  [(set (match_operand:HI 0 "register_operand" "=A")
+	(unspec_volatile:HI [(match_operand 1)] UNS_BUILTIN_MDCL))]
+  ""
+  "movw\t%0, !0xf00e0"
+)
+
+(define_insn "movhi_from_mdch"
+  [(set (match_operand:HI 0 "register_operand" "=A")
+	(unspec_volatile:HI [(match_operand 1)] UNS_BUILTIN_MDCH))]
+  ""
+  "movw\t%0, !0xf00e2"
+)
+
 (define_insn "movqi_to_es"
   [(set (reg:QI ES_REG)
 	(match_operand:QI 0 "register_operand" "a"))]
@@ -51,6 +100,62 @@
   "mov\t%0, es"
 )
 
+(define_insn "movqi_to_mduc"
+  [(set (reg:QI 0 )
+   	(unspec_volatile:QI [(match_operand:QI 0)] UNS_BUILTIN_MDUC))
+			     (match_operand:QI 1 "register_operand" "A")]
+  ""
+  "mov\t!0xf00e8, %1"
+)
+
+(define_insn "movhi_to_mdal"
+  [(set (reg:HI 0 )
+	(unspec_volatile:HI [(match_operand:HI 0)] UNS_BUILTIN_MDAL))
+			     (match_operand:HI 1 "register_operand" "A")]
+  ""
+  "movw\t!0xffff0, %1"
+)
+
+(define_insn "movhi_to_mdah"
+  [(set (reg:HI 0 )
+	(unspec_volatile:HI [(match_operand:HI 0)] UNS_BUILTIN_MDAH))
+			     (match_operand:HI 1 "register_operand" "A")]
+  ""
+  "movw\t!0xffff2, %1"
+)
+
+(define_insn "movhi_to_mdbl"
+  [(set (reg:HI 0 )
+	(unspec_volatile:HI [(match_operand:HI 0)] UNS_BUILTIN_MDBL))
+			     (match_operand:HI 1 "register_operand" "A")]
+  ""
+  "movw\t!0xffff4, %1"
+)
+
+(define_insn "movhi_to_mdbh"
+  [(set (reg:HI 0 )
+	(unspec_volatile:HI [(match_operand:HI 0)] UNS_BUILTIN_MDBH))
+			     (match_operand:HI 1 "register_operand" "A")]
+  ""
+  "movw\t!0xffff6, %1"
+)
+
+(define_insn "movhi_to_mdcl"
+  [(set (reg:HI 0 )
+	(unspec_volatile:HI [(match_operand:HI 0)] UNS_BUILTIN_MDCL))
+			     (match_operand:HI 1 "register_operand" "A")]
+  ""
+  "movw\t!0xf00e0, %1"
+)
+
+(define_insn "movhi_to_mdch"
+  [(set (reg:HI 0 ) 
+	(unspec_volatile:HI [(match_operand:HI 0)] UNS_BUILTIN_MDCH))
+			     (match_operand:HI 1 "register_operand" "A")]
+  ""
+  "movw\t!0xf00e2, %1"
+)
+
 (define_insn "movqi_cs"
   [(set (reg:QI CS_REG)
 	(match_operand:QI 0 "register_operand" "a"))]
Index: gcc/config/rl78/rl78.c
===================================================================
--- gcc/config/rl78/rl78.c	(revision 227024)
+++ gcc/config/rl78/rl78.c	(working copy)
@@ -338,6 +338,10 @@
 #undef  TARGET_OPTION_OVERRIDE
 #define TARGET_OPTION_OVERRIDE		rl78_option_override
 
+#define MUST_SAVE_MDUC_REGISTER                  \
+  (TARGET_SAVE_MDUC_REGISTER                     \
+   && (is_interrupt_func (NULL_TREE)) && RL78_MUL_G13)
+
 static void
 rl78_option_override (void)
 {
@@ -365,6 +369,9 @@
     /* Address spaces are currently only supported by C.  */
     error ("-mes0 can only be used with C");
 
+  if (TARGET_SAVE_MDUC_REGISTER && !(TARGET_G13 || RL78_MUL_G13))
+    warning (0, "mduc registers only saved for G13 target");
+
   switch (rl78_cpu_type)
     {
     case CPU_UNINIT:
@@ -1273,6 +1280,7 @@
   int i, fs;
   rtx sp = gen_rtx_REG (HImode, STACK_POINTER_REGNUM);
   rtx ax = gen_rtx_REG (HImode, AX_REG);
+  rtx operand1;
   int rb = 0;
 
   if (rl78_is_naked_func ())
@@ -1330,6 +1338,39 @@
       F (emit_insn (gen_push (ax)));
     }
 
+  /* Save MDUC register from interrupt routine.  */
+  if (MUST_SAVE_MDUC_REGISTER)
+    {
+      emit_insn (gen_movqi_from_mduc (gen_rtx_REG (QImode, A_REG),
+				      gen_rtx_UNSPEC_VOLATILE (QImode, gen_rtvec (1, operand1),
+							       UNS_BUILTIN_MDUC)));
+      F (emit_insn (gen_push (ax)));
+      emit_insn (gen_movhi_from_mdal (gen_rtx_REG (HImode, AX_REG),
+				      gen_rtx_UNSPEC_VOLATILE (HImode, gen_rtvec (1, operand1),
+				      UNS_BUILTIN_MDAL)));
+      F (emit_insn (gen_push (ax)));
+      emit_insn (gen_movhi_from_mdah (gen_rtx_REG (HImode, AX_REG),
+				      gen_rtx_UNSPEC_VOLATILE (HImode, gen_rtvec (1, operand1),
+				      UNS_BUILTIN_MDAH)));
+      F (emit_insn (gen_push (ax)));
+      emit_insn (gen_movhi_from_mdbl (gen_rtx_REG (HImode, AX_REG),
+				      gen_rtx_UNSPEC_VOLATILE (HImode, gen_rtvec (1, operand1),
+				      UNS_BUILTIN_MDBL)));
+      F (emit_insn (gen_push (ax)));
+      emit_insn (gen_movhi_from_mdbh (gen_rtx_REG (HImode, AX_REG),
+				      gen_rtx_UNSPEC_VOLATILE (HImode, gen_rtvec (1, operand1),
+				      UNS_BUILTIN_MDBH)));
+      F (emit_insn (gen_push (ax)));
+      emit_insn (gen_movhi_from_mdcl (gen_rtx_REG (HImode, AX_REG),
+				      gen_rtx_UNSPEC_VOLATILE (HImode, gen_rtvec (1, operand1),
+				      UNS_BUILTIN_MDCL)));
+      F (emit_insn (gen_push (ax)));
+      emit_insn (gen_movhi_from_mdch (gen_rtx_REG (HImode, AX_REG),
+				      gen_rtx_UNSPEC_VOLATILE (HImode, gen_rtvec (1, operand1),
+				      UNS_BUILTIN_MDCH)));
+      F (emit_insn (gen_push (ax)));
+    }
+
   if (frame_pointer_needed)
     {
       F (emit_move_insn (ax, sp));
@@ -1372,6 +1413,7 @@
   int i, fs;
   rtx sp = gen_rtx_REG (HImode, STACK_POINTER_REGNUM);
   rtx ax = gen_rtx_REG (HImode, AX_REG);
+  rtx operand1;
   int rb = 0;
 
   if (rl78_is_naked_func ())
@@ -1403,6 +1445,39 @@
 	}
     }
 
+  /* Restore MDUC register from interrupt routine  */
+  if (MUST_SAVE_MDUC_REGISTER)
+    {
+      emit_insn (gen_pop (gen_rtx_REG (HImode, AX_REG)));
+      emit_insn (gen_movhi_to_mdch (gen_rtx_UNSPEC_VOLATILE (HImode,
+				    gen_rtvec (1, operand1),UNS_BUILTIN_MDCH),
+				    gen_rtx_REG (HImode, AX_REG)));
+      emit_insn (gen_pop (gen_rtx_REG (HImode, AX_REG)));
+      emit_insn (gen_movhi_to_mdcl (gen_rtx_UNSPEC_VOLATILE (HImode,
+				    gen_rtvec (1, operand1),UNS_BUILTIN_MDCL),
+				    gen_rtx_REG (HImode, AX_REG)));
+      emit_insn (gen_pop (gen_rtx_REG (HImode, AX_REG)));
+      emit_insn (gen_movhi_to_mdbh (gen_rtx_UNSPEC_VOLATILE (HImode,
+				    gen_rtvec (1, operand1),UNS_BUILTIN_MDBH),
+				    gen_rtx_REG (HImode, AX_REG)));
+      emit_insn (gen_pop (gen_rtx_REG (HImode, AX_REG)));
+      emit_insn (gen_movhi_to_mdbl (gen_rtx_UNSPEC_VOLATILE (HImode,
+				    gen_rtvec (1, operand1),UNS_BUILTIN_MDBL),
+				    gen_rtx_REG (HImode, AX_REG)));
+      emit_insn (gen_pop (gen_rtx_REG (HImode, AX_REG)));
+      emit_insn (gen_movhi_to_mdah (gen_rtx_UNSPEC_VOLATILE (HImode,
+				    gen_rtvec (1, operand1),UNS_BUILTIN_MDAH),
+				    gen_rtx_REG (HImode, AX_REG)));
+      emit_insn (gen_pop (gen_rtx_REG (HImode, AX_REG)));
+      emit_insn (gen_movhi_to_mdal (gen_rtx_UNSPEC_VOLATILE (HImode,
+				    gen_rtvec (1, operand1),UNS_BUILTIN_MDAL),
+				    gen_rtx_REG (HImode, AX_REG)));
+      emit_insn (gen_pop (gen_rtx_REG (HImode, AX_REG)));
+      emit_insn (gen_movqi_to_mduc (gen_rtx_UNSPEC_VOLATILE (QImode,
+				    gen_rtvec (1, operand1),UNS_BUILTIN_MDUC),
+				    gen_rtx_REG (QImode, A_REG)));
+    }
+
   if (is_interrupt_func (cfun->decl) && cfun->machine->uses_es)
     {
       emit_insn (gen_pop (gen_rtx_REG (HImode, AX_REG)));
@@ -1498,6 +1573,9 @@
 
   if (cfun->machine->uses_es)
     fprintf (file, "\t; uses ES register\n");
+
+  if (MUST_SAVE_MDUC_REGISTER)
+    fprintf (file, "\t; uses MDUC register\n");
 }
 
 /* Return an RTL describing where a function return value of type RET_TYPE
Index: gcc/config/rl78/rl78.md
===================================================================
--- gcc/config/rl78/rl78.md	(revision 227024)
+++ gcc/config/rl78/rl78.md	(working copy)
@@ -50,6 +50,13 @@
    (UNS_TRAMPOLINE_INIT		20)
    (UNS_TRAMPOLINE_UNINIT	21)
    (UNS_NONLOCAL_GOTO		22)
+   (UNS_BUILTIN_MDUC           35)
+   (UNS_BUILTIN_MDAL           36)
+   (UNS_BUILTIN_MDAH           37)
+   (UNS_BUILTIN_MDBL           38)
+   (UNS_BUILTIN_MDBH           39)
+   (UNS_BUILTIN_MDCL           40)
+   (UNS_BUILTIN_MDCH           41)
 
    ])
 
Index: gcc/config/rl78/rl78.opt
===================================================================
--- gcc/config/rl78/rl78.opt	(revision 227024)
+++ gcc/config/rl78/rl78.opt	(working copy)
@@ -91,3 +91,7 @@
 mes0
 Target Mask(ES0)
 Assume ES is zero throughout program execution, use ES: for read-only data.
+
+msave-mduc-in-interrupts
+Target Mask(SAVE_MDUC_REGISTER)
+Specifies whether interrupt functions should save and restore the MDUC register.
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 227024)
+++ gcc/doc/invoke.texi	(working copy)
@@ -19026,6 +19026,14 @@
 or 32 bits (@option{-m32bit-doubles}) in size.  The default is
 @option{-m32bit-doubles}.
 
+@item -msave-mduc-in-interrupts
+@opindex msave-mduc-in-interrupts
+Specifies that interrupt handler functions should preserve the
+MDUC registers.  This is only necessary if normal code might use
+the MDUC register, 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
+
 @end table
 
 @node RS/6000 and PowerPC Options

-----Original Message-----
From: DJ Delorie [mailto:dj@redhat.com] 
Sent: Friday, June 05, 2015 12:15 PM
To: Kaushik Phatak <Kaushik.Phatak@kpit.com>
Cc: gcc-patches@gcc.gnu.org; nickc@redhat.com
Subject: Re: [PATCH : RL78] Disable interrupts during hardware multiplication routines

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

[Mike Stump]

On Aug 27, 2015, at 6:13 AM, Kaushik Phatak <Kaushik.Phatak@kpit.com> wrote:Hi DJ,
> I have worked out an updated patch, which would save the MDUC specific registers
> in the interrupt routine when the option '-msave-mduc-in-interrupts' is passed.
> This gets active only for the G13 targets.

So, I may have missed the top of this, but it seems to me that the abi should define what is done, and it sure would be nice if the ports followed the abi.

If that can be the case, there shouldn’t be a flag.

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

[DJ Delorie]

> I have worked out an updated patch, which would save the MDUC specific registers
> in the interrupt routine when the option '-msave-mduc-in-interrupts' is passed.
> This gets active only for the G13 targets.

Perhaps we should have both a -msave... and -mno-save... (gcc provides
these by default) with the default if neither is provided, to be "save
if g13"?

As an optimization, gcc could test the interrupt function to see if it
has any multiplcation or call insns, and if not, don't bother with the
(determined to be unneeded) saves.

> +(define_insn "movqi_from_mduc"
> +  [(set (match_operand:QI 0 "register_operand" "=a")
> +(unspec_volatile:QI [(match_operand:QI 1 "" "")] UNS_BUILTIN_MDUC))]
> +  ""
> +  "mov\t%0, !0xf00e8"
> +)

You shouldn't need special move insns for these; they're regular
variables.  Just generate the RTL for the memory references (make sure
they're volatile) and use the standard movhi patterns.

Unspec patterns are typically used for unknown *opcodes*, not special
registers or memory locations.

> @@ -1273,6 +1280,7 @@
>    int i, fs;
>    rtx sp = gen_rtx_REG (HImode, STACK_POINTER_REGNUM);
>    rtx ax = gen_rtx_REG (HImode, AX_REG);
> +  rtx operand1;
>    int rb = 0;
> 
>    if (rl78_is_naked_func ())
> @@ -1330,6 +1338,39 @@
>        F (emit_insn (gen_push (ax)));
>      }
> 
> +  /* Save MDUC register from interrupt routine.  */
> +  if (MUST_SAVE_MDUC_REGISTER)
> +    {
> +      emit_insn (gen_movqi_from_mduc (gen_rtx_REG (QImode, A_REG),
> +      gen_rtx_UNSPEC_VOLATILE (QImode, gen_rtvec (1, operand1),
> +       UNS_BUILTIN_MDUC)));

You never set operand1 to anything.  Since you never use it, you don't
really need it in the patterns either (just put [(const_int 0)] in the
unspec).  (but better to use a regular movhi as described above).

> +as this makes the interrupt handlers faster. The target option -mg13

That last bit is not a complete sentence.

> This message contains information that may be privileged or
> confidential . . .

Reminder that such notices are inappropriate for public mailing lists.

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

[Kaushik Phatak]

[-- Attachment #1: Type: text/plain, Size: 9337 bytes --]

Hi DJ,
Please find attached an updated patch which tries to address the points raised by you in my earlier attempt,
https://gcc.gnu.org/ml/gcc-patches/2015-08/msg01729.html

1. Added an option for -msave.. and -mno-save.. 
   The default will be to save the MDUC registers for the g13 target in the ISR.
   
2. As an optimization, it will check for usage of mul and divmod routines before saving/restoring.

3. I have eliminated the special insns used earlier and directly generating movhi/movqi for the mem reference, also
   setting them as volatile.

4. Updated and fixed the issue in invoke.texi.

This has been regression tested for -mg13 -msim.
The only glitch I observed was the list file printed out the address of the MDUC
registers in decimal and not in HEX, for example,
mov     a, !983272 is displayed instead of,
mov     a, !0xF00E8
However, the objectdump generates these addresses correctly in hex along with their register name 
references (<MDUC>,<MDAL> etc.)

Please let me know if this updated patch is OK.

Best Regards,
Kaushik

gcc/ChangeLog
2015-12-07  Kaushik Phatak  <kaushik.phatak@kpit.com>

	* config/rl78/rl78.c (rl78_expand_prologue): Save the MDUC related
	registers in all interrupt handlers if necessary.
	(rl78_option_override): Add warning.
	(MUST_SAVE_MDUC_REGISTER): New macro.
	(rl78_expand_epilogue): Restore the MDUC registers if necessary.
	* config/rl78/rl78.c (check_mduc_usage): New function.
	* config/rl78/rl78.opt (msave-mduc-in-interrupts): New option.
	(mno-save-mduc-in-interrupts): New option.
	* doc/invoke.texi (@item -msave-mduc-in-interrupts): New item.
	(@item -mno-save-mduc-in-interrupts): New item

Index: gcc/config/rl78/rl78.c
===================================================================
--- gcc/config/rl78/rl78.c	(revision 2871)
+++ gcc/config/rl78/rl78.c	(working copy)
@@ -342,6 +342,10 @@
 #undef  TARGET_OPTION_OVERRIDE
 #define TARGET_OPTION_OVERRIDE		rl78_option_override
 
+#define MUST_SAVE_MDUC_REGISTER                         \
+  (!TARGET_NO_SAVE_MDUC_REGISTER                        \
+   && (is_interrupt_func (NULL_TREE)) && RL78_MUL_G13)
+
 static void
 rl78_option_override (void)
 {
@@ -366,6 +370,9 @@
     /* Address spaces are currently only supported by C.  */
     error ("-mes0 can only be used with C");
 
+  if (TARGET_SAVE_MDUC_REGISTER && !(TARGET_G13 || RL78_MUL_G13))
+    warning (0, "mduc registers only saved for G13 target");
+
   switch (rl78_cpu_type)
     {
     case CPU_UNINIT:
@@ -1307,6 +1314,27 @@
   return (lookup_attribute ("naked", DECL_ATTRIBUTES (current_function_decl)) != NULL_TREE);
 }
 
+/* Check if the block uses mul/div insns.  */
+int
+check_mduc_usage ()
+{
+  rtx insn;
+  basic_block bb;
+  FOR_EACH_BB_FN (bb, cfun)
+  {
+    FOR_BB_INSNS (bb, insn)
+    {
+      if (recog_memoized (insn) == CODE_FOR_udivmodsi4_g13
+          || recog_memoized (insn) == CODE_FOR_mulhi3_g13
+          || recog_memoized (insn) == CODE_FOR_mulsi3_g13)
+        {
+          return 1;
+        }
+    }
+  }
+  return 0;
+}
+
 /* Expand the function prologue (from the prologue pattern).  */
 void
 rl78_expand_prologue (void)
@@ -1318,7 +1346,7 @@
 
   if (rl78_is_naked_func ())
     return;
-
+ 
   /* Always re-compute the frame info - the register usage may have changed.  */
   rl78_compute_frame_info ();
 
@@ -1371,6 +1399,46 @@
       F (emit_insn (gen_push (ax)));
     }
 
+  /* Save MDUC register inside interrupt routine.  */
+  if (MUST_SAVE_MDUC_REGISTER && (!crtl->is_leaf || check_mduc_usage ()))
+    {
+      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)));
+
+      mem_mduc = gen_rtx_MEM (HImode, GEN_INT (0xffff2));
+      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)));
+
+      mem_mduc = gen_rtx_MEM (HImode, GEN_INT (0xffff4));
+      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)));
+
+      mem_mduc = gen_rtx_MEM (HImode, GEN_INT (0xffff6));
+      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)));
+
+      mem_mduc = gen_rtx_MEM (HImode, GEN_INT (0xf00e0));
+      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)));
+
+      mem_mduc = gen_rtx_MEM (HImode, GEN_INT (0xf00e2));
+      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)));
+    }
   if (frame_pointer_needed)
     {
       F (emit_move_insn (ax, sp));
@@ -1444,6 +1512,47 @@
 	}
     }
 
+  /* Restore MDUC register from interrupt routine.  */
+  if (MUST_SAVE_MDUC_REGISTER && (!crtl->is_leaf || check_mduc_usage ()))
+    {
+      rtx mem_mduc;
+
+      mem_mduc = gen_rtx_MEM (HImode, GEN_INT (0xf00e2));
+      MEM_VOLATILE_P (mem_mduc) = 1;
+      emit_insn (gen_pop (gen_rtx_REG (HImode, AX_REG)));
+      emit_insn (gen_movhi (mem_mduc, gen_rtx_REG (HImode, AX_REG)));
+
+      mem_mduc = gen_rtx_MEM (HImode, GEN_INT (0xf00e0));
+      MEM_VOLATILE_P (mem_mduc) = 1;
+      emit_insn (gen_pop (gen_rtx_REG (HImode, AX_REG)));
+      emit_insn (gen_movhi (mem_mduc, gen_rtx_REG (HImode, AX_REG)));
+
+      mem_mduc = gen_rtx_MEM (HImode, GEN_INT (0xffff6));
+      MEM_VOLATILE_P (mem_mduc) = 1;
+      emit_insn (gen_pop (gen_rtx_REG (HImode, AX_REG)));
+      emit_insn (gen_movhi (mem_mduc, gen_rtx_REG (HImode, AX_REG)));
+
+      mem_mduc = gen_rtx_MEM (HImode, GEN_INT (0xffff4));
+      MEM_VOLATILE_P (mem_mduc) = 1;
+      emit_insn (gen_pop (gen_rtx_REG (HImode, AX_REG)));
+      emit_insn (gen_movhi (mem_mduc, gen_rtx_REG (HImode, AX_REG)));
+
+      mem_mduc = gen_rtx_MEM (HImode, GEN_INT (0xffff2));
+      MEM_VOLATILE_P (mem_mduc) = 1;
+      emit_insn (gen_pop (gen_rtx_REG (HImode, AX_REG)));
+      emit_insn (gen_movhi (mem_mduc, gen_rtx_REG (HImode, AX_REG)));
+
+      mem_mduc = gen_rtx_MEM (HImode, GEN_INT (0xffff0));
+      MEM_VOLATILE_P (mem_mduc) = 1;
+      emit_insn (gen_pop (gen_rtx_REG (HImode, AX_REG)));
+      emit_insn (gen_movhi (mem_mduc, gen_rtx_REG (HImode, AX_REG)));
+
+      mem_mduc = gen_rtx_MEM (QImode, GEN_INT (0xf00e8));
+      MEM_VOLATILE_P (mem_mduc) = 1;
+      emit_insn (gen_pop (gen_rtx_REG (HImode, AX_REG)));
+      emit_insn (gen_movqi (mem_mduc, gen_rtx_REG (QImode, A_REG)));
+    }
+
   /* Save ES register inside interrupt functions.  */
   if (is_interrupt_func (cfun->decl) && cfun->machine->uses_es)
     {
@@ -1599,6 +1708,9 @@
 
   if (cfun->machine->uses_es)
     fprintf (file, "\t; uses ES register\n");
+
+  if (MUST_SAVE_MDUC_REGISTER)
+    fprintf (file, "\t; uses MDUC register\n");
 }
 
 /* Return an RTL describing where a function return value of type RET_TYPE
Index: gcc/config/rl78/rl78.opt
===================================================================
--- gcc/config/rl78/rl78.opt	(revision 2871)
+++ gcc/config/rl78/rl78.opt	(working copy)
@@ -103,4 +103,11 @@
 Target Mask(ES0)
 Assume ES is zero throughout program execution, use ES: for read-only data.
 
+msave-mduc-in-interrupts
+Target Mask(SAVE_MDUC_REGISTER)
+Stores the MDUC registers for interrupt handlers for G13 target.
+This is the default.
 
+mno-save-mduc-in-interrupts
+Target RejectNegative Mask(NO_SAVE_MDUC_REGISTER)
+Does not save the MDUC registers for interrupt handlers for G13 target.
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 2871)
+++ gcc/doc/invoke.texi	(working copy)
@@ -18901,6 +18901,18 @@
 registers @code{r24..r31} are reserved for use in interrupt handlers.
 With this option enabled these registers can be used in ordinary
 functions as well.
+
+@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).
 @end table
 
 @node RS/6000 and PowerPC Options


[-- Attachment #2: rl78_mduc_patch.diff --]
[-- Type: application/octet-stream, Size: 7243 bytes --]

Index: gcc/config/rl78/rl78.c
===================================================================
--- gcc/config/rl78/rl78.c	(revision 2871)
+++ gcc/config/rl78/rl78.c	(working copy)
@@ -342,6 +342,10 @@
 #undef  TARGET_OPTION_OVERRIDE
 #define TARGET_OPTION_OVERRIDE		rl78_option_override
 
+#define MUST_SAVE_MDUC_REGISTER                         \
+  (!TARGET_NO_SAVE_MDUC_REGISTER                        \
+   && (is_interrupt_func (NULL_TREE)) && RL78_MUL_G13)
+
 static void
 rl78_option_override (void)
 {
@@ -366,6 +370,9 @@
     /* Address spaces are currently only supported by C.  */
     error ("-mes0 can only be used with C");
 
+  if (TARGET_SAVE_MDUC_REGISTER && !(TARGET_G13 || RL78_MUL_G13))
+    warning (0, "mduc registers only saved for G13 target");
+
   switch (rl78_cpu_type)
     {
     case CPU_UNINIT:
@@ -1307,6 +1314,27 @@
   return (lookup_attribute ("naked", DECL_ATTRIBUTES (current_function_decl)) != NULL_TREE);
 }
 
+/* Check if the block uses mul/div insns.  */
+int
+check_mduc_usage ()
+{
+  rtx insn;
+  basic_block bb;
+  FOR_EACH_BB_FN (bb, cfun)
+  {
+    FOR_BB_INSNS (bb, insn)
+    {
+      if (recog_memoized (insn) == CODE_FOR_udivmodsi4_g13
+          || recog_memoized (insn) == CODE_FOR_mulhi3_g13
+          || recog_memoized (insn) == CODE_FOR_mulsi3_g13)
+        {
+          return 1;
+        }
+    }
+  }
+  return 0;
+}
+
 /* Expand the function prologue (from the prologue pattern).  */
 void
 rl78_expand_prologue (void)
@@ -1371,6 +1399,46 @@
       F (emit_insn (gen_push (ax)));
     }
 
+  /* Save MDUC register inside interrupt routine.  */
+  if (MUST_SAVE_MDUC_REGISTER && (!crtl->is_leaf || check_mduc_usage ()))
+    {
+      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)));
+
+      mem_mduc = gen_rtx_MEM (HImode, GEN_INT (0xffff2));
+      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)));
+
+      mem_mduc = gen_rtx_MEM (HImode, GEN_INT (0xffff4));
+      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)));
+
+      mem_mduc = gen_rtx_MEM (HImode, GEN_INT (0xffff6));
+      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)));
+
+      mem_mduc = gen_rtx_MEM (HImode, GEN_INT (0xf00e0));
+      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)));
+
+      mem_mduc = gen_rtx_MEM (HImode, GEN_INT (0xf00e2));
+      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)));
+    }
   if (frame_pointer_needed)
     {
       F (emit_move_insn (ax, sp));
@@ -1444,6 +1512,47 @@
 	}
     }
 
+  /* Restore MDUC register from interrupt routine.  */
+  if (MUST_SAVE_MDUC_REGISTER && (!crtl->is_leaf || check_mduc_usage ()))
+    {
+      rtx mem_mduc;
+
+      mem_mduc = gen_rtx_MEM (HImode, GEN_INT (0xf00e2));
+      MEM_VOLATILE_P (mem_mduc) = 1;
+      emit_insn (gen_pop (gen_rtx_REG (HImode, AX_REG)));
+      emit_insn (gen_movhi (mem_mduc, gen_rtx_REG (HImode, AX_REG)));
+
+      mem_mduc = gen_rtx_MEM (HImode, GEN_INT (0xf00e0));
+      MEM_VOLATILE_P (mem_mduc) = 1;
+      emit_insn (gen_pop (gen_rtx_REG (HImode, AX_REG)));
+      emit_insn (gen_movhi (mem_mduc, gen_rtx_REG (HImode, AX_REG)));
+
+      mem_mduc = gen_rtx_MEM (HImode, GEN_INT (0xffff6));
+      MEM_VOLATILE_P (mem_mduc) = 1;
+      emit_insn (gen_pop (gen_rtx_REG (HImode, AX_REG)));
+      emit_insn (gen_movhi (mem_mduc, gen_rtx_REG (HImode, AX_REG)));
+
+      mem_mduc = gen_rtx_MEM (HImode, GEN_INT (0xffff4));
+      MEM_VOLATILE_P (mem_mduc) = 1;
+      emit_insn (gen_pop (gen_rtx_REG (HImode, AX_REG)));
+      emit_insn (gen_movhi (mem_mduc, gen_rtx_REG (HImode, AX_REG)));
+
+      mem_mduc = gen_rtx_MEM (HImode, GEN_INT (0xffff2));
+      MEM_VOLATILE_P (mem_mduc) = 1;
+      emit_insn (gen_pop (gen_rtx_REG (HImode, AX_REG)));
+      emit_insn (gen_movhi (mem_mduc, gen_rtx_REG (HImode, AX_REG)));
+
+      mem_mduc = gen_rtx_MEM (HImode, GEN_INT (0xffff0));
+      MEM_VOLATILE_P (mem_mduc) = 1;
+      emit_insn (gen_pop (gen_rtx_REG (HImode, AX_REG)));
+      emit_insn (gen_movhi (mem_mduc, gen_rtx_REG (HImode, AX_REG)));
+
+      mem_mduc = gen_rtx_MEM (QImode, GEN_INT (0xf00e8));
+      MEM_VOLATILE_P (mem_mduc) = 1;
+      emit_insn (gen_pop (gen_rtx_REG (HImode, AX_REG)));
+      emit_insn (gen_movqi (mem_mduc, gen_rtx_REG (QImode, A_REG)));
+    }
+
   /* Save ES register inside interrupt functions.  */
   if (is_interrupt_func (cfun->decl) && cfun->machine->uses_es)
     {
@@ -1599,6 +1708,9 @@
 
   if (cfun->machine->uses_es)
     fprintf (file, "\t; uses ES register\n");
+
+  if (MUST_SAVE_MDUC_REGISTER)
+    fprintf (file, "\t; uses MDUC register\n");
 }
 
 /* Return an RTL describing where a function return value of type RET_TYPE
Index: gcc/config/rl78/rl78.opt
===================================================================
--- gcc/config/rl78/rl78.opt	(revision 2871)
+++ gcc/config/rl78/rl78.opt	(working copy)
@@ -103,4 +103,11 @@
 Target Mask(ES0)
 Assume ES is zero throughout program execution, use ES: for read-only data.
 
+msave-mduc-in-interrupts
+Target Mask(SAVE_MDUC_REGISTER)
+Stores the MDUC registers for interrupt handlers for G13 target.
+This is the default.
 
+mno-save-mduc-in-interrupts
+Target RejectNegative Mask(NO_SAVE_MDUC_REGISTER)
+Does not save the MDUC registers for interrupt handlers for G13 target.
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 2871)
+++ gcc/doc/invoke.texi	(working copy)
@@ -18901,6 +18901,18 @@
 registers @code{r24..r31} are reserved for use in interrupt handlers.
 With this option enabled these registers can be used in ordinary
 functions as well.
+
+@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).
 @end table
 
 @node RS/6000 and PowerPC Options