Re: [patch]: Implement PR104327 for avr

[Richard Biener <richard.guenther@gmail.com>]

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