* [patch]: Implement PR104327 for avr [not found] <7a3552f0-ac4d-754f-a7ba-cba3ff9a4a41@gjlay.de> @ 2023-05-23 12:55 ` Georg-Johann Lay 2023-05-24 9:38 ` Richard Biener 0 siblings, 1 reply; 7+ messages in thread From: Georg-Johann Lay @ 2023-05-23 12:55 UTC (permalink / raw) To: gcc-patches; +Cc: gcc 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); +} + /* 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; \f ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch]: Implement PR104327 for avr 2023-05-23 12:55 ` [patch]: Implement PR104327 for avr Georg-Johann Lay @ 2023-05-24 9:38 ` Richard Biener 2023-05-24 15:44 ` Georg-Johann Lay 0 siblings, 1 reply; 7+ messages in thread From: Richard Biener @ 2023-05-24 9:38 UTC (permalink / raw) To: Georg-Johann Lay; +Cc: gcc-patches, gcc 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; > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch]: Implement PR104327 for avr 2023-05-24 9:38 ` Richard Biener @ 2023-05-24 15:44 ` Georg-Johann Lay 2023-05-25 6:35 ` Richard Biener 0 siblings, 1 reply; 7+ messages in thread From: Georg-Johann Lay @ 2023-05-24 15:44 UTC (permalink / raw) To: Richard Biener; +Cc: gcc-patches, gcc 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; ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch]: Implement PR104327 for avr 2023-05-24 15:44 ` Georg-Johann Lay @ 2023-05-25 6:35 ` Richard Biener 2023-05-25 14:22 ` Georg-Johann Lay 0 siblings, 1 reply; 7+ messages in thread From: Richard Biener @ 2023-05-25 6:35 UTC (permalink / raw) To: Georg-Johann Lay; +Cc: gcc-patches, gcc 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; ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch]: Implement PR104327 for avr 2023-05-25 6:35 ` Richard Biener @ 2023-05-25 14:22 ` Georg-Johann Lay 2023-05-25 15:07 ` Richard Biener 0 siblings, 1 reply; 7+ messages in thread From: Georg-Johann Lay @ 2023-05-25 14:22 UTC (permalink / raw) To: Richard Biener; +Cc: gcc-patches, gcc 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; ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch]: Implement PR104327 for avr 2023-05-25 14:22 ` Georg-Johann Lay @ 2023-05-25 15:07 ` Richard Biener 2023-05-25 17:55 ` [avr,committed]: " Georg-Johann Lay 0 siblings, 1 reply; 7+ messages in thread From: Richard Biener @ 2023-05-25 15:07 UTC (permalink / raw) To: Georg-Johann Lay; +Cc: gcc-patches, gcc > 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; ^ permalink raw reply [flat|nested] 7+ messages in thread
* [avr,committed]: Implement PR104327 for avr 2023-05-25 15:07 ` Richard Biener @ 2023-05-25 17:55 ` Georg-Johann Lay 0 siblings, 0 replies; 7+ messages in thread From: Georg-Johann Lay @ 2023-05-25 17:55 UTC (permalink / raw) To: Richard Biener; +Cc: gcc-patches Am 25.05.23 um 17:07 schrieb Richard Biener: > > >> 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 Committed as https://gcc.gnu.org/r14-1245 Johann --- a/gcc/config/avr/avr.cc +++ b/gcc/config/avr/avr.cc @@ -1018,6 +1018,19 @@ 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 */) +{ + // No restrictions whatsoever. + return true; +} + /* Implement `TARGET_SET_CURRENT_FUNCTION'. */ /* Sanity cheching for above function attributes. */ @@ -14770,6 +14783,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; \f ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-05-25 17:55 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <7a3552f0-ac4d-754f-a7ba-cba3ff9a4a41@gjlay.de> 2023-05-23 12:55 ` [patch]: Implement PR104327 for avr 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 2023-05-25 15:07 ` Richard Biener 2023-05-25 17:55 ` [avr,committed]: " Georg-Johann Lay
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).