Re: -fuse-caller-save - Enable for MIPS
Tom de Vries 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
Re: -fuse-caller-save - Enable for MIPS
Tom de Vries writes:
> On 28-04-14 12:26, Richard Sandiford wrote:
>> Tom de Vries writes:
>>> On 27-04-14 12:27, Richard Sandiford wrote:
Tom de Vries 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
Re: -fuse-caller-save - Enable for MIPS
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
Re: -fuse-caller-save - Enable for MIPS
On 28-04-14 12:26, Richard Sandiford wrote:
Tom de Vries writes:
On 27-04-14 12:27, Richard Sandiford wrote:
Tom de Vries 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
Re: -fuse-caller-save - Enable for MIPS
Tom de Vries writes:
> On 27-04-14 12:27, Richard Sandiford wrote:
>> Tom de Vries 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
Re: -fuse-caller-save - Enable for MIPS
On 27-04-14 12:27, Richard Sandiford wrote:
Tom de Vries writes:
2014-01-12 Radovan Obradovic
Tom de Vries
* 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
2014-01-12 Radovan Obradovic
Tom de Vries
* 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
Re: -fuse-caller-save - Enable for MIPS
Tom de Vries writes:
> 2014-01-12 Radovan Obradovic
> Tom de Vries
>
> * 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.
> @@ -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.
> +/* 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. */
> +/* 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.
I'm a bit surprised that it doesn't work at -O1 for a simple test
like this though. What goes wrong?
Thanks,
Richard
Re: -fuse-caller-save - Enable for MIPS
On 25-04-14 15:22, Richard Sandiford wrote:
Tom de Vries writes:
diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
index 45256e9..b61cd44 100644
--- a/gcc/config/mips/mips.c
+++ b/gcc/config/mips/mips.c
@@ -7027,11 +7027,17 @@ mips_expand_call (enum mips_call_type type, rtx result,
rtx addr,
{
rtx orig_addr, pattern, insn;
int fp_code;
+ rtx post_call_tmp_reg = gen_rtx_REG (word_mode, POST_CALL_TMP_REG);
fp_code = aux == 0 ? 0 : (int) GET_MODE (aux);
insn = mips16_build_call_stub (result, &addr, args_size, fp_code);
if (insn)
{
+ if (TARGET_EXPLICIT_RELOCS
+ && TARGET_CALL_CLOBBERED_GP
+ && !find_reg_note (insn, REG_NORETURN, 0))
+ clobber_reg (&CALL_INSN_FUNCTION_USAGE (insn), post_call_tmp_reg);
+
I think this condition should go in mips_emit_call_insn instead,
so that we don't have 4 instances of it. untyped_call could then
use mips_expand_call as well.
Richard,
Done.
Until now there was no real downside to using $6 for non-MIPS16 code.
Now that there is, it would probably be worth making it:
+#define POST_CALL_TMP_REG \
(TARGET_MIPS16 ? GP_ARG_FIRST + 2 : PIC_OFFSET_TABLE_REGNUM)
and only adding the clobber for MIPS16.
Done.
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 000..1fd6c7d
--- /dev/null
+++ b/gcc/testsuite/gcc.target/mips/fuse-caller-save.c
@@ -0,0 +1,30 @@
+/* { dg-do compile } */
+/* { dg-options "-fuse-caller-save" } */
+/* { dg-skip-if "" { *-*-* } { "*" } { "-Os" } } */
I might have asked this before, sorry, but why this skip? Please add a brief
comment (in the string, if short enough).
I've reduced the amount of skips a bit, and added a comment why they are
skipped.
+/* Testing -fuse-caller-save optimization option. */
+
+static int __attribute__((noinline)) NOCOMPRESSION
+bar (int x)
+{
+ return x + 3;
+}
+
+int __attribute__((noinline)) NOCOMPRESSION
+foo (int y)
+{
+ return y + bar (y);
+}
+
+int NOCOMPRESSION
+main (void)
+{
+ return !(foo (5) == 13);
+}
+
+/* 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" } } */
It'd be good to avoid NOCOMPRESSION because the only case that really
needs the temporary register is MIPS16. Please try putting the test
in a header file and reusing it for three tests, one each of MIPS16,
microMIPS and uncompressed.
Done, I think, I'm not 100% sure I understood what you wanted me to do here.
build and reg-tested on MIPS.
OK for trunk?
Thanks,
- Tom
2014-01-12 Radovan Obradovic
Tom de Vries
* 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.
---
gcc/config/mips/mips-protos.h | 1 +
gcc/config/mips/mips.c | 24 --
gcc/config/mips/mips.h | 7 +++
gcc/config/mips/mips.md| 4 +++-
.../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 +
9 files changed, 98 insertions(+), 7 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 (
Re: -fuse-caller-save - Enable for MIPS
Tom de Vries writes:
> diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
> index 45256e9..b61cd44 100644
> --- a/gcc/config/mips/mips.c
> +++ b/gcc/config/mips/mips.c
> @@ -7027,11 +7027,17 @@ mips_expand_call (enum mips_call_type type, rtx
> result, rtx addr,
> {
>rtx orig_addr, pattern, insn;
>int fp_code;
> + rtx post_call_tmp_reg = gen_rtx_REG (word_mode, POST_CALL_TMP_REG);
>
>fp_code = aux == 0 ? 0 : (int) GET_MODE (aux);
>insn = mips16_build_call_stub (result, &addr, args_size, fp_code);
>if (insn)
> {
> + if (TARGET_EXPLICIT_RELOCS
> + && TARGET_CALL_CLOBBERED_GP
> + && !find_reg_note (insn, REG_NORETURN, 0))
> + clobber_reg (&CALL_INSN_FUNCTION_USAGE (insn), post_call_tmp_reg);
> +
I think this condition should go in mips_emit_call_insn instead,
so that we don't have 4 instances of it. untyped_call could then
use mips_expand_call as well.
Until now there was no real downside to using $6 for non-MIPS16 code.
Now that there is, it would probably be worth making it:
+#define POST_CALL_TMP_REG \
(TARGET_MIPS16 ? GP_ARG_FIRST + 2 : PIC_OFFSET_TABLE_REGNUM)
and only adding the clobber for MIPS16.
> 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 000..1fd6c7d
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/mips/fuse-caller-save.c
> @@ -0,0 +1,30 @@
> +/* { dg-do compile } */
> +/* { dg-options "-fuse-caller-save" } */
> +/* { dg-skip-if "" { *-*-* } { "*" } { "-Os" } } */
I might have asked this before, sorry, but why this skip? Please add a brief
comment (in the string, if short enough).
> +/* Testing -fuse-caller-save optimization option. */
> +
> +static int __attribute__((noinline)) NOCOMPRESSION
> +bar (int x)
> +{
> + return x + 3;
> +}
> +
> +int __attribute__((noinline)) NOCOMPRESSION
> +foo (int y)
> +{
> + return y + bar (y);
> +}
> +
> +int NOCOMPRESSION
> +main (void)
> +{
> + return !(foo (5) == 13);
> +}
> +
> +/* 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" } } */
It'd be good to avoid NOCOMPRESSION because the only case that really
needs the temporary register is MIPS16. Please try putting the test
in a header file and reusing it for three tests, one each of MIPS16,
microMIPS and uncompressed.
Thanks,
Richard
