diff mbox series

[avr,committed] : Implement PR104327 for avr

Message ID 313b5291-80f1-01db-f74c-da7176853eb0@gjlay.de
State New
Headers show
Series [avr,committed] : Implement PR104327 for avr | expand

Commit Message

Georg-Johann Lay May 25, 2023, 5:55 p.m. UTC 
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


  \f
diff mbox series

Patch

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