Re: [patch]: Implement PR104327 for avr

[Georg-Johann Lay <avr@gjlay.de>]

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;