From: "Kewen.Lin" <linkw@linux.ibm.com>
To: David Edelsohn <dje.gcc@gmail.com>,
Segher Boessenkool <segher@kernel.crashing.org>
Cc: Peter Bergner <bergner@linux.ibm.com>,
GCC Patches <gcc-patches@gcc.gnu.org>
Subject: PING^5 [PATCH v2] rs6000: Don't use optimize_function_for_speed_p too early [PR108184]
Date: Mon, 4 Dec 2023 17:57:46 +0800 [thread overview]
Message-ID: <d25f4220-ae9c-b895-0f92-0914c25321db@linux.ibm.com> (raw)
In-Reply-To: <38402e74-14db-9903-3dca-b4ef7c826505@linux.ibm.com>
Hi,
Gentle ping this:
https://gcc.gnu.org/pipermail/gcc-patches/2023-January/609993.html
BR,
Kewen
>
>>>> on 2023/1/16 17:08, Kewen.Lin via Gcc-patches wrote:
>>>>> Hi,
>>>>>
>>>>> As Honza pointed out in [1], the current uses of function
>>>>> optimize_function_for_speed_p in rs6000_option_override_internal
>>>>> are too early, since the query results from the functions
>>>>> optimize_function_for_{speed,size}_p could be changed later due
>>>>> to profile feedback and some function attributes handlings etc.
>>>>>
>>>>> This patch is to move optimize_function_for_speed_p to all the
>>>>> use places of the corresponding flags, which follows the existing
>>>>> practices. Maybe we can cache it somewhere at an appropriate
>>>>> timing, but that's another thing.
>>>>>
>>>>> Comparing with v1[2], this version added one test case for
>>>>> SAVE_TOC_INDIRECT as Segher questioned and suggested, and it
>>>>> also considered the possibility of explicit option (see test
>>>>> cases pr108184-2.c and pr108184-4.c). I believe that excepting
>>>>> for the intentional change on optimize_function_for_{speed,
>>>>> size}_p, there is no other function change.
>>>>>
>>>>> [1] https://gcc.gnu.org/pipermail/gcc-patches/2022-November/607527.html
>>>>> [2] https://gcc.gnu.org/pipermail/gcc-patches/2023-January/609379.html
>>>>>
>>>>> Bootstrapped and regtested on powerpc64-linux-gnu P8,
>>>>> powerpc64le-linux-gnu P{9,10} and powerpc-ibm-aix.
>>>>>
>>>>> Is it ok for trunk?
>>>>>
>>>>> BR,
>>>>> Kewen
>>>>> -----
>>>>> gcc/ChangeLog:
>>>>>
>>>>> * config/rs6000/rs6000.cc (rs6000_option_override_internal): Remove
>>>>> all optimize_function_for_speed_p uses.
>>>>> (fusion_gpr_load_p): Call optimize_function_for_speed_p along
>>>>> with TARGET_P8_FUSION_SIGN.
>>>>> (expand_fusion_gpr_load): Likewise.
>>>>> (rs6000_call_aix): Call optimize_function_for_speed_p along with
>>>>> TARGET_SAVE_TOC_INDIRECT.
>>>>> * config/rs6000/predicates.md (fusion_gpr_mem_load): Call
>>>>> optimize_function_for_speed_p along with TARGET_P8_FUSION_SIGN.
>>>>>
>>>>> gcc/testsuite/ChangeLog:
>>>>>
>>>>> * gcc.target/powerpc/pr108184-1.c: New test.
>>>>> * gcc.target/powerpc/pr108184-2.c: New test.
>>>>> * gcc.target/powerpc/pr108184-3.c: New test.
>>>>> * gcc.target/powerpc/pr108184-4.c: New test.
>>>>> ---
>>>>> gcc/config/rs6000/predicates.md | 5 +++-
>>>>> gcc/config/rs6000/rs6000.cc | 19 +++++++++-----
>>>>> gcc/testsuite/gcc.target/powerpc/pr108184-1.c | 16 ++++++++++++
>>>>> gcc/testsuite/gcc.target/powerpc/pr108184-2.c | 15 +++++++++++
>>>>> gcc/testsuite/gcc.target/powerpc/pr108184-3.c | 25 +++++++++++++++++++
>>>>> gcc/testsuite/gcc.target/powerpc/pr108184-4.c | 24 ++++++++++++++++++
>>>>> 6 files changed, 97 insertions(+), 7 deletions(-)
>>>>> create mode 100644 gcc/testsuite/gcc.target/powerpc/pr108184-1.c
>>>>> create mode 100644 gcc/testsuite/gcc.target/powerpc/pr108184-2.c
>>>>> create mode 100644 gcc/testsuite/gcc.target/powerpc/pr108184-3.c
>>>>> create mode 100644 gcc/testsuite/gcc.target/powerpc/pr108184-4.c
>>>>>
>>>>> diff --git a/gcc/config/rs6000/predicates.md b/gcc/config/rs6000/predicates.md
>>>>> index a1764018545..9f84468db84 100644
>>>>> --- a/gcc/config/rs6000/predicates.md
>>>>> +++ b/gcc/config/rs6000/predicates.md
>>>>> @@ -1878,7 +1878,10 @@ (define_predicate "fusion_gpr_mem_load"
>>>>>
>>>>> /* Handle sign/zero extend. */
>>>>> if (GET_CODE (op) == ZERO_EXTEND
>>>>> - || (TARGET_P8_FUSION_SIGN && GET_CODE (op) == SIGN_EXTEND))
>>>>> + || (TARGET_P8_FUSION_SIGN
>>>>> + && GET_CODE (op) == SIGN_EXTEND
>>>>> + && (rs6000_isa_flags_explicit & OPTION_MASK_P8_FUSION_SIGN
>>>>> + || optimize_function_for_speed_p (cfun))))
>>>>> {
>>>>> op = XEXP (op, 0);
>>>>> mode = GET_MODE (op);
>>>>> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
>>>>> index 6ac3adcec6b..f47d21980a9 100644
>>>>> --- a/gcc/config/rs6000/rs6000.cc
>>>>> +++ b/gcc/config/rs6000/rs6000.cc
>>>>> @@ -3997,8 +3997,7 @@ rs6000_option_override_internal (bool global_init_p)
>>>>> /* If we can shrink-wrap the TOC register save separately, then use
>>>>> -msave-toc-indirect unless explicitly disabled. */
>>>>> if ((rs6000_isa_flags_explicit & OPTION_MASK_SAVE_TOC_INDIRECT) == 0
>>>>> - && flag_shrink_wrap_separate
>>>>> - && optimize_function_for_speed_p (cfun))
>>>>> + && flag_shrink_wrap_separate)
>>>>> rs6000_isa_flags |= OPTION_MASK_SAVE_TOC_INDIRECT;
>>>>>
>>>>> /* Enable power8 fusion if we are tuning for power8, even if we aren't
>>>>> @@ -4032,7 +4031,6 @@ rs6000_option_override_internal (bool global_init_p)
>>>>> zero extending load, and an explicit sign extension. */
>>>>> if (TARGET_P8_FUSION
>>>>> && !(rs6000_isa_flags_explicit & OPTION_MASK_P8_FUSION_SIGN)
>>>>> - && optimize_function_for_speed_p (cfun)
>>>>> && optimize >= 3)
>>>>> rs6000_isa_flags |= OPTION_MASK_P8_FUSION_SIGN;
>>>>>
>>>>> @@ -25690,7 +25688,10 @@ rs6000_call_aix (rtx value, rtx func_desc, rtx tlsarg, rtx cookie)
>>>>>
>>>>> /* Can we optimize saving the TOC in the prologue or
>>>>> do we need to do it at every call? */
>>>>> - if (TARGET_SAVE_TOC_INDIRECT && !cfun->calls_alloca)
>>>>> + if (TARGET_SAVE_TOC_INDIRECT
>>>>> + && !cfun->calls_alloca
>>>>> + && (rs6000_isa_flags_explicit & OPTION_MASK_SAVE_TOC_INDIRECT
>>>>> + || optimize_function_for_speed_p (cfun)))
>>>>> cfun->machine->save_toc_in_prologue = true;
>>>>> else
>>>>> {
>>>>> @@ -27557,7 +27558,10 @@ fusion_gpr_load_p (rtx addis_reg, /* register set via addis. */
>>>>>
>>>>> /* Allow sign/zero extension. */
>>>>> if (GET_CODE (mem) == ZERO_EXTEND
>>>>> - || (GET_CODE (mem) == SIGN_EXTEND && TARGET_P8_FUSION_SIGN))
>>>>> + || (GET_CODE (mem) == SIGN_EXTEND
>>>>> + && TARGET_P8_FUSION_SIGN
>>>>> + && (rs6000_isa_flags_explicit & OPTION_MASK_P8_FUSION_SIGN
>>>>> + || optimize_function_for_speed_p (cfun))))
>>>>> mem = XEXP (mem, 0);
>>>>>
>>>>> if (!MEM_P (mem))
>>>>> @@ -27621,7 +27625,10 @@ expand_fusion_gpr_load (rtx *operands)
>>>>> enum rtx_code extend = UNKNOWN;
>>>>>
>>>>> if (GET_CODE (orig_mem) == ZERO_EXTEND
>>>>> - || (TARGET_P8_FUSION_SIGN && GET_CODE (orig_mem) == SIGN_EXTEND))
>>>>> + || (TARGET_P8_FUSION_SIGN
>>>>> + && GET_CODE (orig_mem) == SIGN_EXTEND
>>>>> + && (rs6000_isa_flags_explicit & OPTION_MASK_P8_FUSION_SIGN
>>>>> + || optimize_function_for_speed_p (cfun))))
>>>>> {
>>>>> extend = GET_CODE (orig_mem);
>>>>> orig_mem = XEXP (orig_mem, 0);
>>>>> diff --git a/gcc/testsuite/gcc.target/powerpc/pr108184-1.c b/gcc/testsuite/gcc.target/powerpc/pr108184-1.c
>>>>> new file mode 100644
>>>>> index 00000000000..eae3bb9b7c4
>>>>> --- /dev/null
>>>>> +++ b/gcc/testsuite/gcc.target/powerpc/pr108184-1.c
>>>>> @@ -0,0 +1,16 @@
>>>>> +/* Only possible to fuse sign extended loads with the addis when
>>>>> + optimize >= 3 and Power8 fusion takes effects. -mno-prefixed
>>>>> + ensures test point isn't broken (due to prefixed plha). */
>>>>> +/* { dg-options "-mdejagnu-tune=power8 -O3 -mno-prefixed" } */
>>>>> +
>>>>> +/* Verify it doesn't optimize this function for speed as it's cold,
>>>>> + by checking it doesn't try to fuse sign extended loads with addis,
>>>>> + which results in a zero extended load and a sign extension. */
>>>>> +
>>>>> +__attribute__ ((cold)) int
>>>>> +fusion_short (short *p)
>>>>> +{
>>>>> + return p[0x12345];
>>>>> +}
>>>>> +
>>>>> +/* { dg-final { scan-assembler-not {\mextsh\M} } } */
>>>>> diff --git a/gcc/testsuite/gcc.target/powerpc/pr108184-2.c b/gcc/testsuite/gcc.target/powerpc/pr108184-2.c
>>>>> new file mode 100644
>>>>> index 00000000000..9f703f7fc6b
>>>>> --- /dev/null
>>>>> +++ b/gcc/testsuite/gcc.target/powerpc/pr108184-2.c
>>>>> @@ -0,0 +1,15 @@
>>>>> +/* -mno-prefixed ensures test point isn't broken (due to prefixed plha). */
>>>>> +/* { dg-options "-mdejagnu-tune=power8 -O2 -mpower8-fusion-sign -mno-prefixed" } */
>>>>> +
>>>>> +/* Verify the explicit option -mpower8-fusion-sign can still fuse
>>>>> + sign extended loads with addis, which results in a zero extended
>>>>> + load and a sign extension. It means the cold attribute which
>>>>> + indicates to optimize for size doesn't affect. */
>>>>> +
>>>>> +__attribute__ ((cold)) int
>>>>> +fusion_short (short *p)
>>>>> +{
>>>>> + return p[0x12345];
>>>>> +}
>>>>> +
>>>>> +/* { dg-final { scan-assembler-times {\mextsh\M} 1 } } */
>>>>> diff --git a/gcc/testsuite/gcc.target/powerpc/pr108184-3.c b/gcc/testsuite/gcc.target/powerpc/pr108184-3.c
>>>>> new file mode 100644
>>>>> index 00000000000..ceaa96e4421
>>>>> --- /dev/null
>>>>> +++ b/gcc/testsuite/gcc.target/powerpc/pr108184-3.c
>>>>> @@ -0,0 +1,25 @@
>>>>> +/* { dg-do compile { target { powerpc*-*-aix* powerpc*-*-linux* } } } */
>>>>> +/* { dg-require-effective-target fpic } */
>>>>> +/* { dg-options "-fpic -mno-pcrel -O2" } */
>>>>> +
>>>>> +/* Verify it doesn't imply -msave-toc-indirect, so
>>>>> + it doesn't take effect and we have two separated
>>>>> + toc savings for these two long calls. */
>>>>> +
>>>>> +void foo (void) __attribute__((__longcall__));
>>>>> +int baz (void) __attribute__((__longcall__));
>>>>> +
>>>>> +__attribute__ ((cold)) int
>>>>> +bar (void)
>>>>> +{
>>>>> + foo ();
>>>>> + return baz () + 1;
>>>>> +}
>>>>> +
>>>>> +/* Linux LE */
>>>>> +/* { dg-final { scan-assembler-times {\mstd 2,24\(1\)} 2 { target powerpc_elfv2 } } } */
>>>>> +/* Linux BE 64 bit or AIX 64 bit */
>>>>> +/* { dg-final { scan-assembler-times {\mstd 2,40\(1\)} 2 { target { lp64 && { ! powerpc_elfv2 } } } } } */
>>>>> +/* AIX 32 bit */
>>>>> +/* { dg-final { scan-assembler-times {\mstw 2,20\(1\)} 2 { target { ilp32 && powerpc*-*-aix* } } } } */
>>>>> +
>>>>> diff --git a/gcc/testsuite/gcc.target/powerpc/pr108184-4.c b/gcc/testsuite/gcc.target/powerpc/pr108184-4.c
>>>>> new file mode 100644
>>>>> index 00000000000..2a70a603047
>>>>> --- /dev/null
>>>>> +++ b/gcc/testsuite/gcc.target/powerpc/pr108184-4.c
>>>>> @@ -0,0 +1,24 @@
>>>>> +/* { dg-do compile { target { powerpc*-*-aix* powerpc*-*-linux* } } } */
>>>>> +/* { dg-require-effective-target fpic } */
>>>>> +/* { dg-options "-fpic -mno-pcrel -O2 -msave-toc-indirect" } */
>>>>> +
>>>>> +/* Verify the explicit -msave-toc-indirect always
>>>>> + takes effect, so there is only one toc saving. */
>>>>> +
>>>>> +void foo (void) __attribute__((__longcall__));
>>>>> +int baz (void) __attribute__((__longcall__));
>>>>> +
>>>>> +__attribute__ ((cold)) int
>>>>> +bar (void)
>>>>> +{
>>>>> + foo ();
>>>>> + return baz () + 1;
>>>>> +}
>>>>> +
>>>>> +/* Linux LE */
>>>>> +/* { dg-final { scan-assembler-times {\mstd 2,24\(1\)} 1 { target powerpc_elfv2 } } } */
>>>>> +/* Linux BE 64 bit or AIX 64 bit */
>>>>> +/* { dg-final { scan-assembler-times {\mstd 2,40\(1\)} 1 { target { lp64 && { ! powerpc_elfv2 } } } } } */
>>>>> +/* AIX 32 bit */
>>>>> +/* { dg-final { scan-assembler-times {\mstw 2,20\(1\)} 1 { target { ilp32 && powerpc*-*-aix* } } } } */
>>>>> +
>>>>> --
>>>>> 2.37.0
next prev parent reply other threads:[~2023-12-04 9:57 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-16 9:08 Kewen.Lin
2023-01-16 9:39 ` [PATCH/RFC] rs6000: Remove optimize_for_speed check for implicit TARGET_SAVE_TOC_INDIRECT [PR108184] Kewen.Lin
2023-01-17 20:57 ` Michael Meissner
2023-01-17 20:58 ` Michael Meissner
2023-01-18 9:04 ` Kewen.Lin
2023-06-15 6:41 ` PING^1 [PATCH v2] rs6000: Don't use optimize_function_for_speed_p too early [PR108184] Kewen.Lin
2023-08-07 10:06 ` PING^2 " Kewen.Lin
2023-10-25 2:48 ` PING^3 " Kewen.Lin
2023-11-08 2:51 ` PING^4 " Kewen.Lin
2023-12-04 9:57 ` Kewen.Lin [this message]
2023-12-12 6:09 ` PING^6 " Kewen.Lin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d25f4220-ae9c-b895-0f92-0914c25321db@linux.ibm.com \
--to=linkw@linux.ibm.com \
--cc=bergner@linux.ibm.com \
--cc=dje.gcc@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=segher@kernel.crashing.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).