From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mo4-p00-ob.smtp.rzone.de (mo4-p00-ob.smtp.rzone.de [85.215.255.21]) by sourceware.org (Postfix) with ESMTPS id DE2083858D32; Thu, 25 May 2023 14:22:46 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org DE2083858D32 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=gjlay.de Authentication-Results: sourceware.org; spf=none smtp.mailfrom=gjlay.de ARC-Seal: i=1; a=rsa-sha256; t=1685024565; cv=none; d=strato.com; s=strato-dkim-0002; b=OV8MWC/yfezhsfzLeojKI1nTCpRjNKmomQ1Uijm/Mvvs8SQkUXRO12FOb7GexWgOqI 0ytfO6NrsSQi9jpUYsK94megxLlAKM7P/0EEeQ3/N8zEWcP3rTU6/0GiDnEGOAA7g0iC RENvh8TKBKvd3gAlwm7FmtZhCTCOWPNDqt/jS8VqtdQh2r8GC03Xz/F6b0YU+zu4iZfJ yztfu2oGHGp4hOCQQlXTtJBDnl4TJerl9URwoBAU+O+1fcBg4a49RCZ+LIM1welU+3y3 vGLGkK9y2EPyWKB5dVmgv4CH6ggj6rlzRGs5kLDsja9lB06/3cJjAgS5Y6Y6ifWuu88O IRhA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; t=1685024565; s=strato-dkim-0002; d=strato.com; h=In-Reply-To:From:References:Cc:To:Subject:Date:Message-ID:Cc:Date: From:Subject:Sender; bh=Wyq721rDcJFWgQA3F81Rl0HKdgRFHpwf7w7lqSIRAIc=; b=ZOU1UFoYAeUlg4L0RE54H4pTIea7S4oIlYUgcmtqwh+OGm49fVz3zKKDFxEkDyb0Rg TlIvjWwpeiyg5sA/Gp87ssHaCAFf6HKhbuiR9boZAW/c8ueufWwwDudC8MDM0aLT+6HP 8nNVSMPU9PlAbMaOLNKRYm0L1lm1lNxGFwlfLJzKDnpJEF52jDvGJ5tyCGftbVeCJHOT TxPd/VCbcKaSOzpE0n0XGnp85r5aNSGi+OlBNLgJEMYYJ06Ope7CbNWAE9bZ/wgbmIo2 zmpKLIW8fF1X+aV169psrvmmQfYN5nxXm/tQfstZr8ybCGSTFGPmPYE/ndRmfLL64BWK XVnQ== ARC-Authentication-Results: i=1; strato.com; arc=none; dkim=none X-RZG-CLASS-ID: mo00 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; t=1685024565; s=strato-dkim-0002; d=gjlay.de; h=In-Reply-To:From:References:Cc:To:Subject:Date:Message-ID:Cc:Date: From:Subject:Sender; bh=Wyq721rDcJFWgQA3F81Rl0HKdgRFHpwf7w7lqSIRAIc=; b=iTuaRITWgql782IXRUE+gNDnhg/KkLG+m7U6WGNMhm0JWq4bcIrKtlnRdT8JK+bhmC nzJOUzb3NUjd+hkJyGU690KtOkO4RTvdUFn8qP/neGaWWwCD+osUDEWw1OUpZYzcY1tE VaeCya0DC3WuWk9yTHHI+NOlvOS5S00q8zRTVRxZeNrinNVcopSKSX4yCgk3s8vEorHS 6inpc0HXu+2BupzgFNHUtLTIAQVo1m7YON2Y1+5MIEUvfPyrxELd+QYiqVKN3JMnU+po uzj2wn822gjy3GhQvrT15egko4K9NRvIMRhsTKc1fuus4jgCOOmHqf3OVBUZXey95pxZ gKeg== DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; t=1685024565; s=strato-dkim-0003; d=gjlay.de; h=In-Reply-To:From:References:Cc:To:Subject:Date:Message-ID:Cc:Date: From:Subject:Sender; bh=Wyq721rDcJFWgQA3F81Rl0HKdgRFHpwf7w7lqSIRAIc=; b=Xcn01WZUWMLlq+9qhp+ydc0on02TE+mriSbESwSnOxehLIAK+QiOH0puReXqiL7i1C OHUr29OdrW7nOL9turCQ== X-RZG-AUTH: ":LXoWVUeid/7A29J/hMvvT3koxZnKT7Qq0xotTetVnKkRmM69o2y+LiO3MutATA==" Received: from [192.168.2.102] by smtp.strato.de (RZmta 49.4.0 DYNA|AUTH) with ESMTPSA id z691f1z4PEMjtox (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256 bits)) (Client did not present a certificate); Thu, 25 May 2023 16:22:45 +0200 (CEST) Message-ID: <72200c1e-a732-0aad-53ca-bcb895e028bb@gjlay.de> Date: Thu, 25 May 2023 16:22:45 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 Subject: Re: [patch]: Implement PR104327 for avr Content-Language: en-US To: Richard Biener Cc: gcc-patches@gcc.gnu.org, gcc@gcc.gnu.org References: <7a3552f0-ac4d-754f-a7ba-cba3ff9a4a41@gjlay.de> <6ecf31c6-1fa2-330d-6744-2a1d013fd530@gjlay.de> From: Georg-Johann Lay In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-8.5 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,SPF_HELO_PASS,SPF_NONE,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.23 um 08:35 schrieb Richard Biener: > On Wed, May 24, 2023 at 5:44 PM Georg-Johann Lay wrote: >> Am 24.05.23 um 11:38 schrieb Richard Biener: >>> On Tue, May 23, 2023 at 2:56 PM 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 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= 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? 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;