* [PATCH : RL78] Disable interrupts during hardware multiplication routines
@ 2015-06-05 6:44 Kaushik Phatak
2015-06-05 8:02 ` DJ Delorie
0 siblings, 1 reply; 13+ messages in thread
From: Kaushik Phatak @ 2015-06-05 6:44 UTC (permalink / raw)
To: 'gcc-patches@gcc.gnu.org'; +Cc: nick clifton (nickc@redhat.com)
[-- 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")]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH : RL78] Disable interrupts during hardware multiplication routines
2015-06-05 6:44 [PATCH : RL78] Disable interrupts during hardware multiplication routines Kaushik Phatak
@ 2015-06-05 8:02 ` DJ Delorie
2015-08-27 13:14 ` Kaushik Phatak
0 siblings, 1 reply; 13+ messages in thread
From: DJ Delorie @ 2015-06-05 8:02 UTC (permalink / raw)
To: Kaushik Phatak; +Cc: gcc-patches, nickc
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH : RL78] Disable interrupts during hardware multiplication routines
2015-06-05 8:02 ` DJ Delorie
@ 2015-08-27 13:14 ` Kaushik Phatak
2015-08-27 17:11 ` Mike Stump
2015-08-27 17:25 ` DJ Delorie
0 siblings, 2 replies; 13+ messages in thread
From: Kaushik Phatak @ 2015-08-27 13:14 UTC (permalink / raw)
To: DJ Delorie
Cc: 'gcc-patches@gcc.gnu.org', nick clifton (nickc@redhat.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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH : RL78] Disable interrupts during hardware multiplication routines
2015-08-27 13:14 ` Kaushik Phatak
@ 2015-08-27 17:11 ` Mike Stump
2015-08-27 17:25 ` DJ Delorie
1 sibling, 0 replies; 13+ messages in thread
From: Mike Stump @ 2015-08-27 17:11 UTC (permalink / raw)
To: Kaushik Phatak; +Cc: DJ Delorie, gcc-patches, nick clifton (nickc@redhat.com)
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH : RL78] Disable interrupts during hardware multiplication routines
2015-08-27 13:14 ` Kaushik Phatak
2015-08-27 17:11 ` Mike Stump
@ 2015-08-27 17:25 ` DJ Delorie
2015-12-07 12:36 ` Kaushik Phatak
1 sibling, 1 reply; 13+ messages in thread
From: DJ Delorie @ 2015-08-27 17:25 UTC (permalink / raw)
To: Kaushik Phatak; +Cc: gcc-patches, nickc
> 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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH : RL78] Disable interrupts during hardware multiplication routines
2015-08-27 17:25 ` DJ Delorie
@ 2015-12-07 12:36 ` Kaushik Phatak
0 siblings, 0 replies; 13+ messages in thread
From: Kaushik Phatak @ 2015-12-07 12:36 UTC (permalink / raw)
To: gcc-patches
[-- 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH : RL78] Disable interrupts during hardware multiplication routines
2016-05-04 11:47 ` Kaushik Phatak
@ 2016-05-09 11:46 ` Nick Clifton
0 siblings, 0 replies; 13+ messages in thread
From: Nick Clifton @ 2016-05-09 11:46 UTC (permalink / raw)
To: Kaushik Phatak, Mike Stump; +Cc: dj, gcc-patches, Kaushik M Phatak
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH : RL78] Disable interrupts during hardware multiplication routines
2016-01-12 15:54 ` Mike Stump
@ 2016-05-04 11:47 ` Kaushik Phatak
2016-05-09 11:46 ` Nick Clifton
0 siblings, 1 reply; 13+ messages in thread
From: Kaushik Phatak @ 2016-05-04 11:47 UTC (permalink / raw)
To: Nick Clifton, Mike Stump; +Cc: dj, gcc-patches, Kaushik M 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH : RL78] Disable interrupts during hardware multiplication routines
2016-01-12 7:21 Kaushik M Phatak
2016-01-12 12:20 ` Nick Clifton
@ 2016-01-12 15:54 ` Mike Stump
2016-05-04 11:47 ` Kaushik Phatak
1 sibling, 1 reply; 13+ messages in thread
From: Mike Stump @ 2016-01-12 15:54 UTC (permalink / raw)
To: Kaushik M Phatak; +Cc: Nick Clifton, dj, gcc-patches, kaushik.phatak
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH : RL78] Disable interrupts during hardware multiplication routines
2016-01-12 7:21 Kaushik M Phatak
@ 2016-01-12 12:20 ` Nick Clifton
2016-01-12 15:54 ` Mike Stump
1 sibling, 0 replies; 13+ messages in thread
From: Nick Clifton @ 2016-01-12 12:20 UTC (permalink / raw)
To: Kaushik M Phatak; +Cc: dj, Mike Stump, gcc-patches, kaushik.phatak
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH : RL78] Disable interrupts during hardware multiplication routines
@ 2016-01-12 7:21 Kaushik M Phatak
2016-01-12 12:20 ` Nick Clifton
2016-01-12 15:54 ` Mike Stump
0 siblings, 2 replies; 13+ messages in thread
From: Kaushik M Phatak @ 2016-01-12 7:21 UTC (permalink / raw)
To: Nick Clifton; +Cc: dj, Mike Stump, gcc-patches, kaushik.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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH : RL78] Disable interrupts during hardware multiplication routines
2016-01-07 11:27 ` Kaushik M Phatak
@ 2016-01-07 12:03 ` Nick Clifton
0 siblings, 0 replies; 13+ messages in thread
From: Nick Clifton @ 2016-01-07 12:03 UTC (permalink / raw)
To: Kaushik M Phatak, dj; +Cc: Mike Stump, gcc-patches, kaushik.phatak
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH : RL78] Disable interrupts during hardware multiplication routines
[not found] ` <C6735D72-E033-470D-A8B2-1949D7A04AEE@comcast.net>
@ 2016-01-07 11:27 ` Kaushik M Phatak
2016-01-07 12:03 ` Nick Clifton
0 siblings, 1 reply; 13+ messages in thread
From: Kaushik M Phatak @ 2016-01-07 11:27 UTC (permalink / raw)
To: Mike Stump, dj, gcc-patches, nickc; +Cc: kaushik.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.
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2016-05-09 11:46 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-06-05 6:44 [PATCH : RL78] Disable interrupts during hardware multiplication routines Kaushik Phatak
2015-06-05 8:02 ` DJ Delorie
2015-08-27 13:14 ` Kaushik Phatak
2015-08-27 17:11 ` Mike Stump
2015-08-27 17:25 ` DJ Delorie
2015-12-07 12:36 ` Kaushik Phatak
[not found] <CAJA2CV1y0Mk20KqgFbdFs19tyPCH7HvR_XGAwy=OyhYcgNLuWQ@mail.gmail.com>
[not found] ` <C6735D72-E033-470D-A8B2-1949D7A04AEE@comcast.net>
2016-01-07 11:27 ` Kaushik M Phatak
2016-01-07 12:03 ` Nick Clifton
2016-01-12 7:21 Kaushik M Phatak
2016-01-12 12:20 ` Nick Clifton
2016-01-12 15:54 ` Mike Stump
2016-05-04 11:47 ` Kaushik Phatak
2016-05-09 11:46 ` Nick Clifton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).