From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ej1-x635.google.com (mail-ej1-x635.google.com [IPv6:2a00:1450:4864:20::635]) by sourceware.org (Postfix) with ESMTPS id 285E33858D32; Thu, 25 May 2023 15:07:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 285E33858D32 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-ej1-x635.google.com with SMTP id a640c23a62f3a-96f6e83e12fso117975966b.1; Thu, 25 May 2023 08:07:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1685027253; x=1687619253; h=to:in-reply-to:cc:references:message-id:date:subject:mime-version :from:content-transfer-encoding:from:to:cc:subject:date:message-id :reply-to; bh=s5DVLA0e3j2zD1Cl3cfeLNRESTFgqjXRmpRt6BIe2So=; b=FoznV81pyMaEDTupVza7Wsb+zkbiAtbu8xv87u1/bvdolwqNK/cdez0gv0+1bzAfbT eL5LY/P5XgM06mm2IHb/iyj4rMRO2PhgiXgjdb1YMO+unABSPiW6YBFgJd8QPpAoSD2i bwn1H5E++yvPQRlg0mP4gbL0mNUtJDdbmpvjTOJ6uqG28e3Ai6F47tCT6VO5He0z02cL KutyM/ecFUeEhLYoTcb8gaJKIl4Go/mydQySp4sYwU1mmvlUQxCIvbOOG5Zdr3zFHgrC yjfbha6bLmBviPCHpAayGErUzjM15qOZz4jOGbEza3GBkKtGXaaSYSOe5KMRkRahGca8 RgFQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1685027253; x=1687619253; h=to:in-reply-to:cc:references:message-id:date:subject:mime-version :from:content-transfer-encoding:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=s5DVLA0e3j2zD1Cl3cfeLNRESTFgqjXRmpRt6BIe2So=; b=M/O3joodbIWWRbVwICFxOs/NxXtz2aV3Xl50mpFHZb9cLiM9wyyKcpSoChVLaGjm/F L75KUfDXVXBRirlYc/Sc6XNuXqBGe7ndN8YYB2JIbWfTvRbl0TUG1A3KydtZkrmXzs/R mRlvXWFvzEJNvijCeyg3fPL8mkuTdZznKaVpBW3M6lAKOJRnrdpCR6WJsvcrlFdB5XyF LNgwgNLqwi3D/VZF/zVXw5GF5ebaafWT4C4uEZItXWZGBQSPMDnlky2hdWWsIo+8r70p vOrm/DHbpKnh5y7dNXdDJcvlBRF6qttCWFbm3rsxwd9SAHVOUclXsPOOr19A3R47yJhb wkUg== X-Gm-Message-State: AC+VfDx5yXbpnXxdl87fI06Pll5/wPZYjKsTrwNFSwhwz0rbfmJ7Kslo xtkwCWm2xTEcy+H1bOApXHuR5REdGos= X-Google-Smtp-Source: ACHHUZ60OWpmlSsH0O7BnJg6xjAr9pbTcRWZyYBtEJWg4ulliHSNhnjLS3PO8NE0NOiy/GSfInEXGg== X-Received: by 2002:a17:907:7f22:b0:973:91f7:508a with SMTP id qf34-20020a1709077f2200b0097391f7508amr2198445ejc.4.1685027252935; Thu, 25 May 2023 08:07:32 -0700 (PDT) Received: from smtpclient.apple (dynamic-095-114-084-028.95.114.pool.telefonica.de. [95.114.84.28]) by smtp.gmail.com with ESMTPSA id s7-20020a170906c30700b0094f410225c7sm929751ejz.169.2023.05.25.08.07.32 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 25 May 2023 08:07:32 -0700 (PDT) Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable From: Richard Biener Mime-Version: 1.0 (1.0) Subject: Re: [patch]: Implement PR104327 for avr Date: Thu, 25 May 2023 17:07:21 +0200 Message-Id: <86AC10B7-7919-4FC7-B36E-7CC2DABE444E@gmail.com> References: <72200c1e-a732-0aad-53ca-bcb895e028bb@gjlay.de> Cc: gcc-patches@gcc.gnu.org, gcc@gcc.gnu.org In-Reply-To: <72200c1e-a732-0aad-53ca-bcb895e028bb@gjlay.de> To: Georg-Johann Lay X-Mailer: iPhone Mail (20F66) X-Spam-Status: No, score=-9.6 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: > Am 25.05.2023 um 16:22 schrieb Georg-Johann Lay : >=20 > =EF=BB=BF >=20 >> Am 25.05.23 um 08:35 schrieb Richard Biener: >>> On Wed, May 24, 2023 at 5:44=E2=80=AFPM Georg-Johann Lay w= rote: >>> 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: >>>>>=20 >>>>> 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. >>>>>=20 >>>>> Proposing the following patch that implements TARGET_CAN_INLINE_P. >>>>>=20 >>>>> Ok to apply? >>>>>=20 >>>>> Johann >>>>>=20 >>>>> target/104327: Allow more inlining between different optimization leve= ls. >>>>>=20 >>>>> 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. >>>>>=20 >>>>> 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"); >>>>> } >>>>>=20 >>>>> + >>>>> +/* 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); >>>>=20 >>>> I'm not sure if AVR has ISA extensions but the above will likely break >>>> things like >>>>=20 >>>> void __attribute__((target("-mX"))) foo () { asm ("isa X opcode"); >>>> stmt-that-generates-X-ISA; } >>>=20 >>> This yields >>>=20 >>> 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 ta= rget >> 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 (); >>>> } >>>>=20 >>>> if always-inlines are the concern you can use >>>>=20 >>>> 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; >>>>=20 >>>> return default_target_can_inline_p (caller, callee); >>>=20 >>> 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). >=20 > Ok, simply "true" sounds reasonable. Is that change ok then? Yes. Richard=20 > Johann >=20 >=20 >> Richard. >>> Johann >>>=20 >>>>> +} >>>>> + >>>>> /* Implement `TARGET_SET_CURRENT_FUNCTION'. */ >>>>> /* Sanity cheching for above function attributes. */ >>>>>=20 >>>>> @@ -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 >>>>>=20 >>>>> +#undef TARGET_CAN_INLINE_P >>>>> +#define TARGET_CAN_INLINE_P avr_can_inline_p >>>>> + >>>>> struct gcc_target targetm =3D TARGET_INITIALIZER;