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

[Kaushik Phatak <Kaushik.Phatak@kpit.com>]

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