diff mbox

[:,RL78] Disable interrupts during hardware multiplication routines

Message ID HK2PR03MB138012713E3B01220DD3E2DEFC6F0@HK2PR03MB1380.apcprd03.prod.outlook.com
State New
Headers show

Commit Message

Kaushik Phatak Aug. 27, 2015, 1:13 p.m. UTC 
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.
	

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

Comments

Mike Stump Aug. 27, 2015, 4:53 p.m. UTC | #1 
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.
DJ Delorie Aug. 27, 2015, 5:11 p.m. UTC | #2 
> 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.
diff mbox

Patch

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