diff mbox

-fuse-caller-save - Enable for MIPS

Message ID 535D94EE.6080704@mentor.com
State New
Headers show

Commit Message

Tom de Vries April 27, 2014, 11:38 p.m. UTC 
On 27-04-14 12:27, Richard Sandiford wrote:
> Tom de Vries <Tom_deVries@mentor.com> writes:
>> 2014-01-12  Radovan Obradovic  <robradovic@mips.com>
>>              Tom de Vries  <tom@codesourcery.com>
>>
>> 	* config/mips/mips-protos.h (mips_emit_call_insn): Declare.
>> 	* config/mips/mips.h (POST_CALL_TMP_REG): Define.
>> 	* config/mips/mips.c (mips_emit_call_insn): Remove static.  Use
>> 	last_call_insn.  Add POST_CALL_TMP_REG clobber
>> 	 (mips_split_call): Use POST_CALL_TMP_REG.
>> 	(TARGET_CALL_FUSAGE_CONTAINS_NON_CALLEE_CLOBBERS): Redefine to true.
>> 	* config/mips/mips.md (define_expand "untyped_call"): Use
>> 	mips_emit_call_insn.
>>
>> 	* gcc.target/mips/mips.exp: Add use-caller-save to -ffoo/-fno-foo
>> 	options.
>> 	* gcc.target/mips/fuse-caller-save.h: New include file.
>> 	* gcc.target/mips/fuse-caller-save.c: New test.
>> 	* gcc.target/mips/fuse-caller-save-mips16.c: Same.
>> 	* gcc.target/mips/fuse-caller-save-micromips.c: Same.
>
> Sorry, a couple of things, but this is looking pretty good:
>
>>   mips_emit_call_insn (rtx pattern, rtx orig_addr, rtx addr, bool lazy_p)
>>   {
>>     rtx insn, reg;
>>
>> -  insn = emit_call_insn (pattern);
>> +  emit_call_insn (pattern);
>> +  insn = last_call_insn ();
>>
>>     if (TARGET_MIPS16 && mips_use_pic_fn_addr_reg_p (orig_addr))
>>       {
>
> This change isn't necessary; emit_call_insn is defined to return a CALL_INSN.
>

I dropped this change, as well as the change in the untyped_call expand, I 
realized it's unnecessary.

>> @@ -2843,6 +2844,16 @@ mips_emit_call_insn (rtx pattern, rtx orig_addr, rtx addr, bool lazy_p)
>>   	       gen_rtx_REG (Pmode, GOT_VERSION_REGNUM));
>>         emit_insn (gen_update_got_version ());
>>       }
>> +
>> +  if (TARGET_MIPS16
>> +      && TARGET_EXPLICIT_RELOCS
>> +      && TARGET_CALL_CLOBBERED_GP
>> +      && !find_reg_note (insn, REG_NORETURN, 0))
>> +    {
>> +      rtx post_call_tmp_reg = gen_rtx_REG (word_mode, POST_CALL_TMP_REG);
>> +      clobber_reg (&CALL_INSN_FUNCTION_USAGE (insn), post_call_tmp_reg);
>> +    }
>
> The REG_NORETURN note won't be around yet, so we might as well drop
> that line.  I'm not sure how useful it would be anyway since values
> are never live across a noreturn call.
>

Done.

>> +/* Temporary register that is used after a call.  $4 and $5 are used for
>
> Might as well make it "...used when restoring $gp after a call", now that
> it's not as obvious from context.
>
>> +   returning complex double values in soft-float code, so $6 is the first
>> +   suitable candidate for !TARGET_MIPS16.  For TARGET_MIPS16, we use
>> +   PIC_OFFSET_TABLE_REGNUM instead.  */
>
> !TARGET_MIPS16 and TARGET_MIPS16 are the wrong way around:
>
>     suitable candidate for TARGET_MIPS16.  For !TARGET_MIPS16 we can use
>     $gp itself as the temporary.  */
>

Fixed, thanks for catching that.

>> +/* The scan of the sp-relative saves will fail for -O0 and -O1.
>> +   For -flto, scans will fail because there's no code in the .s file.  */
>> +/* { dg-skip-if "" { *-*-* }  { "-O0" "-O1" "-flto"} } */
>
> The -flto thing is handled automatically by the testsuite
> (see force_conventional_output_for) so that one should be left out.
>

Ah, I see. Removed.

> I'm a bit surprised that it doesn't work at -O1 for a simple test
> like this though.  What goes wrong?
>

AFAIU now the problem is that the optimization doesn't trigger for -O0 and -01, 
because the register allocator behaves more conservatively.

OK for trunk, if re-testing succeeds?

Thanks,
- Tom

Comments

Richard Sandiford April 28, 2014, 10:26 a.m. UTC | #1 
Tom de Vries <Tom_deVries@mentor.com> writes:
> On 27-04-14 12:27, Richard Sandiford wrote:
>> Tom de Vries <Tom_deVries@mentor.com> writes:
>>>   mips_emit_call_insn (rtx pattern, rtx orig_addr, rtx addr, bool lazy_p)
>>>   {
>>>     rtx insn, reg;
>>>
>>> -  insn = emit_call_insn (pattern);
>>> +  emit_call_insn (pattern);
>>> +  insn = last_call_insn ();
>>>
>>>     if (TARGET_MIPS16 && mips_use_pic_fn_addr_reg_p (orig_addr))
>>>       {
>>
>> This change isn't necessary; emit_call_insn is defined to return a CALL_INSN.
>>
>
> I dropped this change, as well as the change in the untyped_call expand, I 
> realized it's unnecessary.

Why was the untyped_call part unnecessary?

>> I'm a bit surprised that it doesn't work at -O1 for a simple test
>> like this though.  What goes wrong?
>>
>
> AFAIU now the problem is that the optimization doesn't trigger for -O0
> and -01, because the register allocator behaves more conservatively.

Hmm, is that just because -fcaller-saves is -O2 and above?  If so,
should -fuse-caller-save imply -fcaller-saves?

Thanks,
Richard
Tom de Vries April 28, 2014, 10:47 a.m. UTC | #2 
On 28-04-14 12:26, Richard Sandiford wrote:
> Tom de Vries <Tom_deVries@mentor.com> writes:
>> On 27-04-14 12:27, Richard Sandiford wrote:
>>> Tom de Vries <Tom_deVries@mentor.com> writes:
>>>>    mips_emit_call_insn (rtx pattern, rtx orig_addr, rtx addr, bool lazy_p)
>>>>    {
>>>>      rtx insn, reg;
>>>>
>>>> -  insn = emit_call_insn (pattern);
>>>> +  emit_call_insn (pattern);
>>>> +  insn = last_call_insn ();
>>>>
>>>>      if (TARGET_MIPS16 && mips_use_pic_fn_addr_reg_p (orig_addr))
>>>>        {
>>>
>>> This change isn't necessary; emit_call_insn is defined to return a CALL_INSN.
>>>
>>
>> I dropped this change, as well as the change in the untyped_call expand, I
>> realized it's unnecessary.
>
> Why was the untyped_call part unnecessary?
>

The define_expand "untyped_call" uses GEN_CALL, which uses define_expand "call", 
which uses mips_expand_call, which uses mips_emit_call_insn, which adds the 
required clobbers.

>>> I'm a bit surprised that it doesn't work at -O1 for a simple test
>>> like this though.  What goes wrong?
>>>
>>
>> AFAIU now the problem is that the optimization doesn't trigger for -O0
>> and -01, because the register allocator behaves more conservatively.
>
> Hmm, is that just because -fcaller-saves is -O2 and above?
>  If so,
> should -fuse-caller-save imply -fcaller-saves?
>
> Thanks,
> Richard
>
Tom de Vries April 28, 2014, 12:10 p.m. UTC | #3 
On 28-04-14 12:47, Tom de Vries wrote:
> Hmm, is that just because -fcaller-saves is -O2 and above?

For -O1, after adding -fcaller-saves the optimization triggers, and the 
test-cases passes.

For -O0, adding -fcaller-saves doesn't make a difference, the optimization 
doesn't trigger.

> If so,
> should -fuse-caller-save imply -fcaller-saves?

I don't think it's strictly necessary, but I can make a patch if required.

Thanks,
- Tom
Richard Sandiford April 28, 2014, 2:53 p.m. UTC | #4 
Tom de Vries <Tom_deVries@mentor.com> writes:
> On 28-04-14 12:26, Richard Sandiford wrote:
>> Tom de Vries <Tom_deVries@mentor.com> writes:
>>> On 27-04-14 12:27, Richard Sandiford wrote:
>>>> Tom de Vries <Tom_deVries@mentor.com> writes:
>>>>>    mips_emit_call_insn (rtx pattern, rtx orig_addr, rtx addr, bool lazy_p)
>>>>>    {
>>>>>      rtx insn, reg;
>>>>>
>>>>> -  insn = emit_call_insn (pattern);
>>>>> +  emit_call_insn (pattern);
>>>>> +  insn = last_call_insn ();
>>>>>
>>>>>      if (TARGET_MIPS16 && mips_use_pic_fn_addr_reg_p (orig_addr))
>>>>>        {
>>>>
>>>> This change isn't necessary; emit_call_insn is defined to return a
>>>> CALL_INSN.
>>>>
>>>
>>> I dropped this change, as well as the change in the untyped_call expand, I
>>> realized it's unnecessary.
>>
>> Why was the untyped_call part unnecessary?
>>
>
> The define_expand "untyped_call" uses GEN_CALL, which uses
> define_expand "call", which uses mips_expand_call, which uses
> mips_emit_call_insn, which adds the required clobbers.

Ah, yeah.  In that case please keep mips_emit_call_insn static.

OK with that change, although please remove -O1 if the agreement
is that "-O1 -fuse-call-save" should work.

Thanks,
Richard
Richard Sandiford April 28, 2014, 2:55 p.m. UTC | #5 
Tom de Vries <Tom_deVries@mentor.com> writes:
>> If so,
>> should -fuse-caller-save imply -fcaller-saves?
>
> I don't think it's strictly necessary, but I can make a patch if required.

Implying -fcaller-saves seems better to me, since "-O -fuse-caller-save"
looks like it should enable the new optimisation.  It's not my call though.

Thanks,
Richard
diff mbox

Patch

2014-01-12  Radovan Obradovic  <robradovic@mips.com>
            Tom de Vries  <tom@codesourcery.com>

	* config/mips/mips-protos.h (mips_emit_call_insn): Declare.
	* config/mips/mips.h (POST_CALL_TMP_REG): Define.
	* config/mips/mips.c (mips_emit_call_insn): Remove static.  Add
	POST_CALL_TMP_REG clobber.
	(mips_split_call): Use POST_CALL_TMP_REG.
	(TARGET_CALL_FUSAGE_CONTAINS_NON_CALLEE_CLOBBERS): Redefine to true.

	* gcc.target/mips/mips.exp: Add use-caller-save to -ffoo/-fno-foo
	options.
	* gcc.target/mips/fuse-caller-save.h: New include file.
	* gcc.target/mips/fuse-caller-save.c: New test.
	* gcc.target/mips/fuse-caller-save-mips16.c: Same.
	* gcc.target/mips/fuse-caller-save-micromips.c: Same.
---
 gcc/config/mips/mips-protos.h                        |  1 +
 gcc/config/mips/mips.c                               | 20 +++++++++++++++-----
 gcc/config/mips/mips.h                               |  7 +++++++
 .../gcc.target/mips/fuse-caller-save-micromips.c     | 17 +++++++++++++++++
 .../gcc.target/mips/fuse-caller-save-mip16.c         | 17 +++++++++++++++++
 gcc/testsuite/gcc.target/mips/fuse-caller-save.c     | 17 +++++++++++++++++
 gcc/testsuite/gcc.target/mips/fuse-caller-save.h     | 17 +++++++++++++++++
 gcc/testsuite/gcc.target/mips/mips.exp               |  1 +
 8 files changed, 92 insertions(+), 5 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/mips/fuse-caller-save-micromips.c
 create mode 100644 gcc/testsuite/gcc.target/mips/fuse-caller-save-mip16.c
 create mode 100644 gcc/testsuite/gcc.target/mips/fuse-caller-save.c
 create mode 100644 gcc/testsuite/gcc.target/mips/fuse-caller-save.h

diff --git a/gcc/config/mips/mips-protos.h b/gcc/config/mips/mips-protos.h
index 3d59b7b..19ea919 100644
--- a/gcc/config/mips/mips-protos.h
+++ b/gcc/config/mips/mips-protos.h
@@ -232,6 +232,7 @@  extern bool mips_use_pic_fn_addr_reg_p (const_rtx);
 extern rtx mips_expand_call (enum mips_call_type, rtx, rtx, rtx, rtx, bool);
 extern void mips_split_call (rtx, rtx);
 extern bool mips_get_pic_call_symbol (rtx *, int);
+extern rtx mips_emit_call_insn (rtx, rtx, rtx, bool);
 extern void mips_expand_fcc_reload (rtx, rtx, rtx);
 extern void mips_set_return_address (rtx, rtx);
 extern bool mips_move_by_pieces_p (unsigned HOST_WIDE_INT, unsigned int);
diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
index 45256e9..b80c1b4 100644
--- a/gcc/config/mips/mips.c
+++ b/gcc/config/mips/mips.c
@@ -2814,7 +2814,7 @@  mips_force_temporary (rtx dest, rtx value)
    ADDR is the legitimized form, and LAZY_P is true if the call
    address is lazily-bound.  */
 
-static rtx
+rtx
 mips_emit_call_insn (rtx pattern, rtx orig_addr, rtx addr, bool lazy_p)
 {
   rtx insn, reg;
@@ -2843,6 +2843,15 @@  mips_emit_call_insn (rtx pattern, rtx orig_addr, rtx addr, bool lazy_p)
 	       gen_rtx_REG (Pmode, GOT_VERSION_REGNUM));
       emit_insn (gen_update_got_version ());
     }
+
+  if (TARGET_MIPS16
+      && TARGET_EXPLICIT_RELOCS
+      && TARGET_CALL_CLOBBERED_GP)
+    {
+      rtx post_call_tmp_reg = gen_rtx_REG (word_mode, POST_CALL_TMP_REG);
+      clobber_reg (&CALL_INSN_FUNCTION_USAGE (insn), post_call_tmp_reg);
+    }
+
   return insn;
 }
 
@@ -7099,10 +7108,8 @@  mips_split_call (rtx insn, rtx call_pattern)
 {
   emit_call_insn (call_pattern);
   if (!find_reg_note (insn, REG_NORETURN, 0))
-    /* Pick a temporary register that is suitable for both MIPS16 and
-       non-MIPS16 code.  $4 and $5 are used for returning complex double
-       values in soft-float code, so $6 is the first suitable candidate.  */
-    mips_restore_gp_from_cprestore_slot (gen_rtx_REG (Pmode, GP_ARG_FIRST + 2));
+    mips_restore_gp_from_cprestore_slot (gen_rtx_REG (Pmode,
+						      POST_CALL_TMP_REG));
 }
 
 /* Return true if a call to DECL may need to use JALX.  */
@@ -19134,6 +19141,9 @@  mips_atomic_assign_expand_fenv (tree *hold, tree *clear, tree *update)
 #undef TARGET_ATOMIC_ASSIGN_EXPAND_FENV
 #define TARGET_ATOMIC_ASSIGN_EXPAND_FENV mips_atomic_assign_expand_fenv
 
+#undef TARGET_CALL_FUSAGE_CONTAINS_NON_CALLEE_CLOBBERS
+#define TARGET_CALL_FUSAGE_CONTAINS_NON_CALLEE_CLOBBERS true
+
 struct gcc_target targetm = TARGET_INITIALIZER;
 
 #include "gt-mips.h"
diff --git a/gcc/config/mips/mips.h b/gcc/config/mips/mips.h
index b25865b..14fa8fd 100644
--- a/gcc/config/mips/mips.h
+++ b/gcc/config/mips/mips.h
@@ -2212,6 +2212,13 @@  enum reg_class
 #define FP_ARG_FIRST (FP_REG_FIRST + 12)
 #define FP_ARG_LAST  (FP_ARG_FIRST + MAX_ARGS_IN_REGISTERS - 1)
 
+/* Temporary register that is used when restoring $gp after a call.  $4 and $5
+   are used for returning complex double values in soft-float code, so $6 is the
+   first suitable candidate for TARGET_MIPS16.  For !TARGET_MIPS16 we can use
+   $gp itself as the temporary.  */
+#define POST_CALL_TMP_REG \
+  (TARGET_MIPS16 ? GP_ARG_FIRST + 2 : PIC_OFFSET_TABLE_REGNUM)
+
 /* 1 if N is a possible register number for function argument passing.
    We have no FP argument registers when soft-float.  When FP registers
    are 32 bits, we can't directly reference the odd numbered ones.  */
diff --git a/gcc/testsuite/gcc.target/mips/fuse-caller-save-micromips.c b/gcc/testsuite/gcc.target/mips/fuse-caller-save-micromips.c
new file mode 100644
index 0000000..6ad01c7
--- /dev/null
+++ b/gcc/testsuite/gcc.target/mips/fuse-caller-save-micromips.c
@@ -0,0 +1,17 @@ 
+/* { dg-do compile } */
+/* { dg-options "-fuse-caller-save (-mmicromips)" } */
+/* At -O0 and -O1, the register allocator behaves more conservatively, and
+   the fuse-caller-save optimization doesnt' trigger.  */
+/* { dg-skip-if "" { *-*-* }  { "-O0" "-O1" } } */
+/* Testing -fuse-caller-save optimization option.  */
+
+#define ATTRIBUTE MICROMIPS
+#include "fuse-caller-save.h"
+
+/* Check that there are only 2 stack-saves: r31 in main and foo.  */
+
+/* Check that there only 2 sw/sd.  */
+/* { dg-final { scan-assembler-times "(?n)s\[wd\]\t\\\$.*,.*\\(\\\$sp\\)" 2 } } */
+
+/* Check that the first caller-save register is unused.  */
+/* { dg-final { scan-assembler-not "\\\$16" } } */
diff --git a/gcc/testsuite/gcc.target/mips/fuse-caller-save-mip16.c b/gcc/testsuite/gcc.target/mips/fuse-caller-save-mip16.c
new file mode 100644
index 0000000..a7c6cf4
--- /dev/null
+++ b/gcc/testsuite/gcc.target/mips/fuse-caller-save-mip16.c
@@ -0,0 +1,17 @@ 
+/* { dg-do compile } */
+/* { dg-options "-fuse-caller-save (-mips16)" } */
+/* At -O0 and -O1, the register allocator behaves more conservatively, and
+   the fuse-caller-save optimization doesnt' trigger.  */
+/* { dg-skip-if "" { *-*-* }  { "-O0" "-O1" } } */
+/* Testing -fuse-caller-save optimization option.  */
+
+#define ATTRIBUTE MIPS16
+#include "fuse-caller-save.h"
+
+/* Check that there are only 2 stack-saves: r31 in main and foo.  */
+
+/* Check that there only 2 sw/sd.  */
+/* { dg-final { scan-assembler-times "(?n)s\[wd\]\t\\\$.*,.*\\(\\\$sp\\)" 2 } } */
+
+/* Check that the first caller-save register is unused.  */
+/* { dg-final { scan-assembler-not "\\\$16" } } */
diff --git a/gcc/testsuite/gcc.target/mips/fuse-caller-save.c b/gcc/testsuite/gcc.target/mips/fuse-caller-save.c
new file mode 100644
index 0000000..72c08fe
--- /dev/null
+++ b/gcc/testsuite/gcc.target/mips/fuse-caller-save.c
@@ -0,0 +1,17 @@ 
+/* { dg-do compile } */
+/* { dg-options "-fuse-caller-save" } */
+/* At -O0 and -O1, the register allocator behaves more conservatively, and
+   the fuse-caller-save optimization doesnt' trigger.  */
+/* { dg-skip-if "" { *-*-* }  { "-O0" "-O1" } } */
+/* Testing -fuse-caller-save optimization option.  */
+
+#define ATTRIBUTE NOCOMPRESSION
+#include "fuse-caller-save.h"
+
+/* Check that there are only 2 stack-saves: r31 in main and foo.  */
+
+/* Check that there only 2 sw/sd.  */
+/* { dg-final { scan-assembler-times "(?n)s\[wd\]\t\\\$.*,.*\\(\\\$sp\\)" 2 } } */
+
+/* Check that the first caller-save register is unused.  */
+/* { dg-final { scan-assembler-not "\\\$16" } } */
diff --git a/gcc/testsuite/gcc.target/mips/fuse-caller-save.h b/gcc/testsuite/gcc.target/mips/fuse-caller-save.h
new file mode 100644
index 0000000..edf6039
--- /dev/null
+++ b/gcc/testsuite/gcc.target/mips/fuse-caller-save.h
@@ -0,0 +1,17 @@ 
+static int __attribute__((noinline)) ATTRIBUTE
+bar (int x)
+{
+  return x + 3;
+}
+
+int __attribute__((noinline)) ATTRIBUTE
+foo (int y)
+{
+  return y + bar (y);
+}
+
+int ATTRIBUTE
+main (void)
+{
+  return !(foo (5) == 13);
+}
diff --git a/gcc/testsuite/gcc.target/mips/mips.exp b/gcc/testsuite/gcc.target/mips/mips.exp
index 8c72cff..6ad8160 100644
--- a/gcc/testsuite/gcc.target/mips/mips.exp
+++ b/gcc/testsuite/gcc.target/mips/mips.exp
@@ -305,6 +305,7 @@  foreach option {
     tree-vectorize
     unroll-all-loops
     unroll-loops
+    use-caller-save
 } {
     lappend mips_option_groups $option "-f(no-|)$option"
 }
-- 
1.8.3.2