Message ID | 6ecf31c6-1fa2-330d-6744-2a1d013fd530@gjlay.de |
---|---|
State | New |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id D1E883855583 for <patchwork@sourceware.org>; Tue, 23 May 2023 12:56:05 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mo4-p00-ob.smtp.rzone.de (mo4-p00-ob.smtp.rzone.de [81.169.146.216]) by sourceware.org (Postfix) with ESMTPS id 7920A3858284; Tue, 23 May 2023 12:55:47 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 7920A3858284 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=gjlay.de Authentication-Results: sourceware.org; spf=none smtp.mailfrom=gjlay.de ARC-Seal: i=1; a=rsa-sha256; t=1684846546; cv=none; d=strato.com; s=strato-dkim-0002; b=o57cxCl7CTrFYRQTOo+nlAq1UGiZWhnF/Ci3hcaiN81zdpwayueMx7WtLhviH5Dftw zPoaVYrlt6y7KNLp/xZs6PZkBHOVzgGnJMSyCd7Ocs6NtCAg3AXSxA153lSMO3nDWAa4 Lkx3FgbqaUeec2aZPlSzut84OZRDioJIMd/wNxev+gXAiggnE9rz9AChQrIPHSdgOm69 x/AkKlPb1a4zrLbD7mputvFYeH8I+RmnWVp/HCmTqNKGXKFuTrG67XNiY5Wjh/RuPfeS EDr2WM1ZaFwTm2S8PPpMhzKf62AJitum4gtZTCTXOW8u1fDUUv+Z40hoRSQbY1ZK/kiy fPDw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; t=1684846546; s=strato-dkim-0002; d=strato.com; h=In-Reply-To:Cc:References:To:From:Subject:Date:Message-ID:Cc:Date: From:Subject:Sender; bh=1LdweywbIsoN53h6Ltb2RjeNvrW0hNt7BZrH0KuHXj8=; b=GLjn8sMhwwvdxB+dfMQ1QDrCRc+psXnhnlN7G+ZqT/Kw72agLskBU18hceXIajYTxV VJ49+aqnR4PsB8cAFRBaTAOndLtWYBbHemg4Wud14e2XwqtGF0Ogi0a0IPVwbzM4fESu 4m4ojtwhN/7Kqq7YcJkfOpGvISfgqDqJU4LZzpvW/NIdYbSDc2YXRjPOydlM4EGBLeTm p8/rHC8nxbUbf1vcadVZXHWrpxBFkTqndbf9w/irJWpMCZkXJFGLmoOEL90ZevOc+i3k V6dIHl0mzFLE9qh5nr/Q6qPeZuQfDgM4Q0JZ0f4pT6tHyiVwMljoQn8FLFh2+GhUYRIJ Hr0g== ARC-Authentication-Results: i=1; strato.com; arc=none; dkim=none X-RZG-CLASS-ID: mo00 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; t=1684846546; s=strato-dkim-0002; d=gjlay.de; h=In-Reply-To:Cc:References:To:From:Subject:Date:Message-ID:Cc:Date: From:Subject:Sender; bh=1LdweywbIsoN53h6Ltb2RjeNvrW0hNt7BZrH0KuHXj8=; b=Hcth5mL/53Ttt9TSIQ7B/nND6ADmWfeJP+f9RiBZMk1a+CJds61QW/fMjYhr0LsAWe VJgjXAmIk/i3AyYBkvzHpO5N6q8FdsLNBjDwTGlJ8LAk2WP7yZHPvz1b1hYViUk1GXxj UgHc7nQBqLLwd9W+r5DuckIkN10K2n2zbi7Nf/uzx3mUTWT4jatDumvRARmMTozkY0+3 58sW0pxugPXGBgDrGrxkq8Tp6QEVgmjmexMS60gYQ8g3VTxQcuudpT1JcmOlQ5EN1DN3 ULfAtfGTRgmjgDHZmMIIXbmuOvX+d5jZEf9YZ5uTjwn68EZ3gfeKKOa6HkmPdnvfKbAf 4UOQ== DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; t=1684846546; s=strato-dkim-0003; d=gjlay.de; h=In-Reply-To:Cc:References:To:From:Subject:Date:Message-ID:Cc:Date: From:Subject:Sender; bh=1LdweywbIsoN53h6Ltb2RjeNvrW0hNt7BZrH0KuHXj8=; b=rI+ROz/kbGNcgHtRM/K4cTK9AvVqntknEZHhPdbv5njkzNnjIiiOMcKf66Nxbq2sGE b+53Si/HH0pTT5IJG7Ag== X-RZG-AUTH: ":LXoWVUeid/7A29J/hMvvT3koxZnKT7Qq0xotTetVnKkRmM69o2y+LiO3MutATA==" Received: from [192.168.2.102] by smtp.strato.de (RZmta 49.4.0 DYNA|AUTH) with ESMTPSA id z691f1z4NCtjfbv (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256 bits)) (Client did not present a certificate); Tue, 23 May 2023 14:55:45 +0200 (CEST) Message-ID: <6ecf31c6-1fa2-330d-6744-2a1d013fd530@gjlay.de> Date: Tue, 23 May 2023 14:55:45 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 Subject: [patch]: Implement PR104327 for avr Content-Language: en-US From: Georg-Johann Lay <avr@gjlay.de> To: gcc-patches@gcc.gnu.org References: <7a3552f0-ac4d-754f-a7ba-cba3ff9a4a41@gjlay.de> Cc: gcc@gcc.gnu.org In-Reply-To: <7a3552f0-ac4d-754f-a7ba-cba3ff9a4a41@gjlay.de> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-9.4 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, SPF_HELO_PASS, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE, URIBL_BLACK autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
: Implement PR104327 for avr
|
|
Commit Message
Georg-Johann Lay
May 23, 2023, 12:55 p.m. UTC
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.
Comments
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; } 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); > +} > + > /* 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; > > >
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] 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. > 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? So I need to take into account inlining costs and decide on that whether it's preferred to inline a function or not? 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;
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). 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;
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;
> Am 25.05.2023 um 16:22 schrieb Georg-Johann Lay <avr@gjlay.de>: > > > >> 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? Yes. Richard > 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;
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); +} + /* 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;