diff mbox

[:,RL78] Disable interrupts during hardware multiplication routines

Message ID MA1PR01MB023126DA4F90B1DB96987EA9FC7B0@MA1PR01MB0231.INDPRD01.PROD.OUTLOOK.COM
State New
Headers show

Commit Message

Kaushik Phatak May 4, 2016, 11:47 a.m. UTC
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.




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

Comments

Nick Clifton May 9, 2016, 11:45 a.m. UTC | #1
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
diff mbox

Patch

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