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.pha...@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:ni...@redhat.com]
Sent: Thursday, January 07, 2016 5:33 PM
To: Kaushik M Phatak <kpha...@gmail.com>; d...@redhat.com
Cc: Mike Stump <mikest...@comcast.net>; gcc-patches@gcc.gnu.org;
Kaushik Phatak <kaushik.pha...@kpit.com>
Subject: Re: [PATCH : RL78] Disable interrupts during hardware
multiplication routines
Hi Kaushik,
Just a few comments on the patch itself:
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
