From: Georg-Johann Lay <avr@gjlay.de>
To: Richard Biener <richard.guenther@gmail.com>
Cc: gcc-patches@gcc.gnu.org, gcc@gcc.gnu.org
Subject: Re: [patch]: Implement PR104327 for avr
Date: Thu, 25 May 2023 16:22:45 +0200 [thread overview]
Message-ID: <72200c1e-a732-0aad-53ca-bcb895e028bb@gjlay.de> (raw)
In-Reply-To: <CAFiYyc0SiLER8R3_RfyHh7BQ+hOvKr9WPi_=WqgxqYmewB8kGA@mail.gmail.com>
Am 25.05.23 um 08:35 schrieb Richard Biener:
> On Wed, May 24, 2023 at 5:44 PM Georg-Johann Lay <avr@gjlay.de> wrote:
>> Am 24.05.23 um 11:38 schrieb Richard Biener:
>>> On Tue, May 23, 2023 at 2:56 PM Georg-Johann Lay <avr@gjlay.de> wrote:
>>>>
>>>> PR target/104327 not only affects s390 but also avr:
>>>> The avr backend pre-sets some options depending on optimization level.
>>>> The inliner then thinks that always_inline functions are not eligible
>>>> for inlining and terminates with an error.
>>>>
>>>> Proposing the following patch that implements TARGET_CAN_INLINE_P.
>>>>
>>>> Ok to apply?
>>>>
>>>> Johann
>>>>
>>>> target/104327: Allow more inlining between different optimization levels.
>>>>
>>>> avr-common.cc introduces the following options that are set depending
>>>> on optimization level: -mgas-isr-prologues, -mmain-is-OS-task and
>>>> -fsplit-wide-types-early. The inliner thinks that different options
>>>> disallow cross-optimization inlining, so provide can_inline_p.
>>>>
>>>> gcc/
>>>> PR target/104327
>>>> * config/avr/avr.cc (avr_can_inline_p): New static function.
>>>> (TARGET_CAN_INLINE_P): Define to that function.
>>>> diff --git a/gcc/config/avr/avr.cc b/gcc/config/avr/avr.cc
>>>> index 9fa50ca230d..55b48f63865 100644
>>>> --- a/gcc/config/avr/avr.cc
>>>> +++ b/gcc/config/avr/avr.cc
>>>> @@ -1018,6 +1018,22 @@ avr_no_gccisr_function_p (tree func)
>>>> return avr_lookup_function_attribute1 (func, "no_gccisr");
>>>> }
>>>>
>>>> +
>>>> +/* Implement `TARGET_CAN_INLINE_P'. */
>>>> +/* Some options like -mgas_isr_prologues depend on optimization level,
>>>> + and the inliner might think that due to different options, inlining
>>>> + is not permitted; see PR104327. */
>>>> +
>>>> +static bool
>>>> +avr_can_inline_p (tree /* caller */, tree callee)
>>>> +{
>>>> + // For now, dont't allow to inline ISRs. If the user actually wants
>>>> + // to inline ISR code, they have to turn the body of the ISR into an
>>>> + // ordinary function.
>>>> +
>>>> + return ! avr_interrupt_function_p (callee);
>>>
>>> I'm not sure if AVR has ISA extensions but the above will likely break
>>> things like
>>>
>>> void __attribute__((target("-mX"))) foo () { asm ("isa X opcode");
>>> stmt-that-generates-X-ISA; }
>>
>> This yields
>>
>> warning: target attribute is not supported on this machine [-Wattributes]
>
> Ah, that's an interesting fact. So that indeed leaves
> __attribute__((optimize(...)))
> influencing the set of active target attributes via the generic option target
> hooks like in your case the different defaults.
>
>> avr has -mmcu=<arch> target options, but switching them in mid-air
>> won't work because the file prologue might already be different
>> and incompatible across different architectures. And I never
>> saw any user requesting such a thing, and I can't imagine
>> any reasonable use case... If the warning is not strong enough,
>> may be it can be turned into an error, but -Wattributes is not
>> specific enough for that.
>
> Note the target attribute is then simply ignored.
>
>>> void bar ()
>>> {
>>> if (cpu-has-X)
>>> foo ();
>>> }
>>>
>>> if always-inlines are the concern you can use
>>>
>>> bool always_inline
>>> = (DECL_DISREGARD_INLINE_LIMITS (callee)
>>> && lookup_attribute ("always_inline",
>>> DECL_ATTRIBUTES (callee)));
>>> /* Do what the user says. */
>>> if (always_inline)
>>> return true;
>>>
>>> return default_target_can_inline_p (caller, callee);
>>
>> The default implementation of can_inline_p worked fine for avr.
>> As far as I understand, the new behavior is due to clean-up
>> of global states for options?
>
> I think the last change was r8-2658-g9b25e12d2d940a which
> for targets without target attribute support made it more likely
> to run into the default hook actually comparing the options.
> Previously the "default" was oddly special-cased but you
> could have still run into compares with two different set of
> defaults when there's another "default" default. Say, compile
> with -O2 and have one optimize(0) and one optimize(Os)
> function it would compare the optimize(0) and optimize(Os)
> set if they were distinct from the -O2 set. That probably never
> happened for AVR.
>
>> So I need to take into account inlining costs and decide on that
>> whether it's preferred to inline a function or not?
>
> No, the hook isn't about cost, it's about full incompatibility. So
> if the different -m options that could be in effect for AVR in
> a single TU for different functions never should prevent inlining
> then simply make the hook return true. If there's a specific
> option (that can differ from what specified on the compiler
> command line!) that should, then you should compare the
> setting of that option from the DECL_FUNCTION_SPECIFIC_TARGET
> of the caller and the callee.
>
> But as far as I can see simply returning true should be correct
> for AVR, or like your patch handle interrupts differently (though
> the -Winline diagnostic will tell the user there's a mismatch in
> target options which might be confusing).
Ok, simply "true" sounds reasonable. Is that change ok then?
Johann
> Richard.
>
>> Johann
>>
>>>> +}
>>>> +
>>>> /* Implement `TARGET_SET_CURRENT_FUNCTION'. */
>>>> /* Sanity cheching for above function attributes. */
>>>>
>>>> @@ -14713,6 +14729,9 @@ avr_float_lib_compare_returns_bool (machine_mode
>>>> mode, enum rtx_code)
>>>> #undef TARGET_MD_ASM_ADJUST
>>>> #define TARGET_MD_ASM_ADJUST avr_md_asm_adjust
>>>>
>>>> +#undef TARGET_CAN_INLINE_P
>>>> +#define TARGET_CAN_INLINE_P avr_can_inline_p
>>>> +
>>>> struct gcc_target targetm = TARGET_INITIALIZER;
next prev parent reply other threads:[~2023-05-25 14:22 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <7a3552f0-ac4d-754f-a7ba-cba3ff9a4a41@gjlay.de>
2023-05-23 12:55 ` Georg-Johann Lay
2023-05-24 9:38 ` Richard Biener
2023-05-24 15:44 ` Georg-Johann Lay
2023-05-25 6:35 ` Richard Biener
2023-05-25 14:22 ` Georg-Johann Lay [this message]
2023-05-25 15:07 ` Richard Biener
2023-05-25 17:55 ` [avr,committed]: " Georg-Johann Lay
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=72200c1e-a732-0aad-53ca-bcb895e028bb@gjlay.de \
--to=avr@gjlay.de \
--cc=gcc-patches@gcc.gnu.org \
--cc=gcc@gcc.gnu.org \
--cc=richard.guenther@gmail.com \
/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).