[patch]: Implement PR104327 for avr

[patch]: Implement PR104327 for avr

[Georg-Johann Lay]

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

Re: [patch]: Implement PR104327 for avr

[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; }

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;
>
>
>

Re: [patch]: Implement PR104327 for avr

[Georg-Johann Lay]

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;

Re: [patch]: Implement PR104327 for avr

[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).

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;

Re: [patch]: Implement PR104327 for avr

[Georg-Johann Lay]

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;

Re: [patch]: Implement PR104327 for avr

[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 

> 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;

[avr,committed]: Implement PR104327 for avr

[Georg-Johann Lay]

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