From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lj1-x232.google.com (mail-lj1-x232.google.com [IPv6:2a00:1450:4864:20::232]) by sourceware.org (Postfix) with ESMTPS id 2EA183858D28; Thu, 25 May 2023 06:38:00 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 2EA183858D28 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-lj1-x232.google.com with SMTP id 38308e7fff4ca-2af29b37bd7so2151891fa.1; Wed, 24 May 2023 23:38:00 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1684996678; x=1687588678; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=GgWU0nBPqG9dowTLW0f0EFP0ujlK3IKiZKN18IW3qVs=; b=OGohcmT4I+R7RvxrFxMtp1zRwiKjSfV2+dHgN+xiE1w9r133TMTXpfUSCpTlwXTtdE f6kutWPq0RnWexyXrgDsU6HynCcKromZp6veegghWs3lgyq6hMAaWjmWuK4kAut7eHrD EXKTZjTPoG7Wj8R7W7G+IUbF85KHwvYrupa4nhCZjRyzno1hNImingOJtX20v8LD665P qJ8gA9FJzrQ+IR0OgySB9vZi4J5v+cZSfVF4lH3yk6eDpbm5USawSKM+D2+6OIfGBbLs QQTgoCQgBuKjE515HVuANQWgtrTxuju2g3LOYC2hAPKAaivJCxQoCHflnmT/jNKLDRsT GfmA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1684996678; x=1687588678; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=GgWU0nBPqG9dowTLW0f0EFP0ujlK3IKiZKN18IW3qVs=; b=RAKaWNaOT9dTEt6OiegBwHO2cqXM+9tNiT+9bOy/XktQgsxYiCr7gBoV53nZ8rEo9y ewsZClM5zEOcHRp8ASBWbu0SKufVCFIx1xETn8mY9YOMBrgiwHo2/S92+y58CPy74PVE AkKTwJAh/zWxmHvJL7UIK6TAs32pujiUDt+97NKpO0LDYzu2sqX0S5W+k7BvFKn0vJF+ FX6BAmgszYeJYEE0u1hwDiT718HT2IGhlwpE28LBXokw1BkOJ+F4xpLCWrTBx/Ga2pkh P27jQMGOjlRlr18LnN+xnvO/jmgxhARbjGDrXAXt5qlxYK8Dtw1KUYOMPEjqP7D7NZuJ 6POA== X-Gm-Message-State: AC+VfDwlBMncxWLVZnPfuDo2unD2pi8I+k8lv0XVaCFlqmjCX0ct8E5k QtF3SxN3g9M+A1AoUmHb6Tdk3BlmJEWHLzs2/FsByRYCRco= X-Google-Smtp-Source: ACHHUZ6Sm7Bzajjge030a2UJF1mm8bRNSVf2+cjXJOSYss8WB/AdHjA9v5d7pkoiON+jdnEu/oZZBNMXZPtJrJ6vO7c= X-Received: by 2002:a2e:9ad0:0:b0:2af:1a67:d52 with SMTP id p16-20020a2e9ad0000000b002af1a670d52mr834948ljj.37.1684996678393; Wed, 24 May 2023 23:37:58 -0700 (PDT) MIME-Version: 1.0 References: <7a3552f0-ac4d-754f-a7ba-cba3ff9a4a41@gjlay.de> <6ecf31c6-1fa2-330d-6744-2a1d013fd530@gjlay.de> In-Reply-To: From: Richard Biener Date: Thu, 25 May 2023 08:35:51 +0200 Message-ID: Subject: Re: [patch]: Implement PR104327 for avr To: Georg-Johann Lay Cc: gcc-patches@gcc.gnu.org, gcc@gcc.gnu.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-6.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE,URIBL_BLACK autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Wed, May 24, 2023 at 5:44=E2=80=AFPM Georg-Johann Lay wro= te: > > > > Am 24.05.23 um 11:38 schrieb Richard Biener: > > On Tue, May 23, 2023 at 2:56=E2=80=AFPM Georg-Johann Lay = 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 leve= ls. > >> > >> 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, inlinin= g > >> + 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 want= s > >> + // to inline ISR code, they have to turn the body of the ISR into a= n > >> + // 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 targ= et hooks like in your case the different defaults. > avr has -mmcu=3D 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 > > =3D (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_mo= de > >> 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 =3D TARGET_INITIALIZER;