inlining failed in call to 'always_inline': target specific option mismatch

inlining failed in call to 'always_inline': target specific option mismatch

[Georg-Johann Lay]

This error pops up in the testsuite for avr.

As far as I understand, this is due to target-specific optimization like 
in avr-common.cc:

     { OPT_LEVELS_1_PLUS_NOT_DEBUG, OPT_mgas_isr_prologues, NULL, 1 },
     { OPT_LEVELS_1_PLUS, OPT_mmain_is_OS_task, NULL, 1 },
     // Stick to the "old" placement of the subreg lowering pass.
     { OPT_LEVELS_1_PLUS, OPT_fsplit_wide_types_early, NULL, 1 },

My question is how to fix this.

The backend does not implement can_inline_p, and adding "Optimization" 
to the respective option definition in avr.opt does not help.

Johann

[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: inlining failed in call to 'always_inline': target specific option mismatch

[Richard Biener]

On Tue, May 23, 2023 at 1:25 PM Georg-Johann Lay <avr@gjlay.de> wrote:
>
> This error pops up in the testsuite for avr.
>
> As far as I understand, this is due to target-specific optimization like
> in avr-common.cc:
>
>      { OPT_LEVELS_1_PLUS_NOT_DEBUG, OPT_mgas_isr_prologues, NULL, 1 },
>      { OPT_LEVELS_1_PLUS, OPT_mmain_is_OS_task, NULL, 1 },
>      // Stick to the "old" placement of the subreg lowering pass.
>      { OPT_LEVELS_1_PLUS, OPT_fsplit_wide_types_early, NULL, 1 },

For testcases that have say __attribute__(optimize(0))?  Do you have a specific
example?

> My question is how to fix this.
>
> The backend does not implement can_inline_p, and adding "Optimization"
> to the respective option definition in avr.opt does not help.

I think you'd need to implement the target hook and at least handle
the mismatches of target options you want to allow for inlining,
otherwise the default implementations rejects any mismatch, so
for example when a function has -mgas-isr-prologues but a callee
has not it will never inline that (not even with always-inline I think).

Richard.

> Johann

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: inlining failed in call to 'always_inline': target specific option mismatch

[Georg-Johann Lay]

Am 24.05.23 um 11:28 schrieb Richard Biener:
> On Tue, May 23, 2023 at 1:25 PM Georg-Johann Lay <avr@gjlay.de> wrote:
>>
>> This error pops up in the testsuite for avr.
>>
>> As far as I understand, this is due to target-specific optimization like
>> in avr-common.cc:
>>
>>       { OPT_LEVELS_1_PLUS_NOT_DEBUG, OPT_mgas_isr_prologues, NULL, 1 },
>>       { OPT_LEVELS_1_PLUS, OPT_mmain_is_OS_task, NULL, 1 },
>>       // Stick to the "old" placement of the subreg lowering pass.
>>       { OPT_LEVELS_1_PLUS, OPT_fsplit_wide_types_early, NULL, 1 },
> 
> For testcases that have say __attribute__(optimize(0))?  Do you have a specific
> example?

It's gcc.c-torture/compile/pr104327.c I came across this when checking
fallout.  That test case occurred for a target, but was put in generic
space so other targets would be notified, too:

/* PR target/104327 */

void foo (int *);

static inline __attribute__((always_inline)) void
bar (int *x)
{
   foo (x);
}

__attribute__((cold, optimize("Os"))) void
baz (int *x)
{
   bar (x);
}

Johann


>> My question is how to fix this.
>>
>> The backend does not implement can_inline_p, and adding "Optimization"
>> to the respective option definition in avr.opt does not help.
> 
> I think you'd need to implement the target hook and at least handle
> the mismatches of target options you want to allow for inlining,

As I wrote in the other answer, setting target options in attribute
is nor supported.  Target attributes should be diagnosed or even
throw error.

Johann

> otherwise the default implementations rejects any mismatch, so
> for example when a function has -mgas-isr-prologues but a callee
> has not it will never inline that (not even with always-inline I think).

It's actually find to inline anything, anywhere.  I added the condition
on interrupts because they get special prologues. Inlining ISR code in
other functions would work, but its not graat style.

Johann

> 
> Richard.

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;