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
