Am 25.05.23 um 17:07 schrieb Richard Biener:
Am 25.05.2023 um 16:22 schrieb Georg-Johann Lay <a...@gjlay.de>:
Am 25.05.23 um 08:35 schrieb Richard Biener:
On Wed, May 24, 2023 at 5:44 PM Georg-Johann Lay <a...@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 <a...@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
