From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ed1-x535.google.com (mail-ed1-x535.google.com [IPv6:2a00:1450:4864:20::535]) by sourceware.org (Postfix) with ESMTPS id E83E6389365C for ; Thu, 22 Apr 2021 08:27:23 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org E83E6389365C Received: by mail-ed1-x535.google.com with SMTP id i3so26708390edt.1 for ; Thu, 22 Apr 2021 01:27:23 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=8Cmio9N3vv0bNydNrBjyzJeusAS65dffzDV/LIjYWus=; b=Mo0g8pfj3Cu4NlpaYzYBpMJIfkKUBAUeRtwsB82FqPosRMeL2sHT3EUUAZg5BvMUC7 sM+1ZE7xNVTcy3E+AzMn2M34Od2Jc6SyUMxPGQGyYJMm9ewYZFMiVTdYqCVMgyLXl//p 53jvEDwAVWvCUI8k8QuVqBs1n97x6xd9/oeL32sh4NULiymzb/OeAyZFzr0Uga2vh3pt hw6BleyrvsRfKINccFdD6XuqgIbT3PcdKRQqLAcVU9Gazkteds0rzkzWk0zZTtCOfFxV IWsbMrlxBIMjuyK4rRHysRkemp6f3Pm2RqMmg3AYbOBpsy7Xb7elSGZhXIJpxk2nyolp lsyg== X-Gm-Message-State: AOAM532BhPOh5iSEz12NSVc9SnvhR7MF5V63MLcG+YPbRGQS5e85r2KD 9CsWoROZHK5lvult+CmubMpSVE27eerSzaGs6aA= X-Google-Smtp-Source: ABdhPJyHerlxYLM0YGciZmv20JLQ/c9x7QNzm6VGDBSnhOH99EJZOZgtKQOGj9I/65AcNO8qJYBJQa+ij06zkHDWRPU= X-Received: by 2002:a05:6402:354b:: with SMTP id f11mr2277662edd.361.1619080042887; Thu, 22 Apr 2021 01:27:22 -0700 (PDT) MIME-Version: 1.0 References: <20210414223918.230495-1-hjl.tools@gmail.com> <20210414223918.230495-3-hjl.tools@gmail.com> In-Reply-To: From: Richard Biener Date: Thu, 22 Apr 2021 10:27:11 +0200 Message-ID: Subject: Re: [PATCH v4 2/2] x86: Add general_regs_only function attribute To: "H.J. Lu" Cc: Martin Sebor , GCC Patches , Uros Bizjak , Jakub Jelinek , Bernhard Reutner-Fischer Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 22 Apr 2021 08:27:27 -0000 On Thu, Apr 22, 2021 at 3:01 AM H.J. Lu wrote: > > On Wed, Apr 21, 2021 at 4:24 PM Martin Sebor wrote: > > > > On 4/21/21 2:58 PM, H.J. Lu wrote: > > > On Wed, Apr 21, 2021 at 10:09 AM Martin Sebor wrote: > > >> > > >> On 4/14/21 4:39 PM, H.J. Lu wrote: > > >>> commit 87c753ac241f25d222d46ba1ac66ceba89d6a200 > > >>> Author: H.J. Lu > > >>> Date: Fri Aug 21 09:42:49 2020 -0700 > > >>> > > >>> x86: Add target("general-regs-only") function attribute > > >>> > > >>> is incomplete since it is impossible to call integer intrinsics from > > >>> a function with general-regs-only target attribute. > > >>> > > >>> 1. Add general_regs_only function attribute to inform the compiler that > > >>> functions use only general purpose registers. When making inlining > > >>> decisions on such functions, non-GPR compiler options are excluded. > > >>> 2. Add general_regs_only attribute to x86 intrinsics which use only > > >>> general purpose registers. > > >>> > > >> ... > > >>> --- a/gcc/doc/extend.texi > > >>> +++ b/gcc/doc/extend.texi > > >>> @@ -7066,6 +7066,11 @@ On x86 targets, the @code{fentry_section} attribute sets the name > > >>> of the section to record function entry instrumentation calls in when > > >>> enabled with @option{-pg -mrecord-mcount} > > >>> > > >>> +@item general_regs_only > > >>> +@cindex @code{general_regs_only} function attribute, x86 > > >>> +The @code{general_regs_only} attribute on functions is used to > > >>> +inform the compiler that functions use only general purpose registers. > > >> > > >> I'll just reiterate basically the same comment as before: it's not > > >> clear from the very brief description above what the requirements > > >> are for using the attribute. I'm guessing it can be applied to > > >> any function (inline or otherwise) but only has any effect when > > >> the function is actually inlined and otherwise doesn't constrain > > >> what the function can do. (Whatever the constraints are, I think > > >> the manual should spell them out, and likewise for its effects.) > > > > > > That is correct. > > > > > >> Similarly it's not clear what should be expected when the function > > >> does use some other register. Ideally, I think GCC would check and > > >> issue a nice error message whether or not the function is inlined > > >> or called. I suspect that might only be possible for inline > > >> functions that are actually called and for which the back end must > > >> emit code. > > > > > > This is what GCC does today: > > > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99744 > > > > Yes, that's the rather obscure error I think I commented on before > > and suggested should be improved. Based on r99744-3.c I don't think > > this has changed in the improved patch. > > My goal is to fix the inline failures, not to improve the compiler error > message. > > > > > > >> Other than that, I'd suggest to improve the phrasing a bit: > > >> > > >> The @code{general_regs_only} function attribute indicates that > > >> the function uses only general purpose registers... [text > > >> explaining constraints and errors follows]. > > >> > > >> Martin > > > > > > How about this > > > > > > @item general_regs_only > > > @cindex @code{general_regs_only} function attribute, x86 > > > The @code{general_regs_only} attribute on functions is used to inform > > > the compiler that functions use only general purpose registers. It > > > can be used together with the @code{always_inline} attribute to avoid > > > inlining failure when there is a mismatch in compiler vector options. > > > > Without an article the part "that functions use only general purpose > > registers" is unclear and/or grammatically incorrect. What functions? > > If the function the attribute is applied to, it needs an article, e.g., > > "the function" or "a function", and singular. (Otherwise it could be > > read as talking about the functions called from the one with > > the attribute, or some other functions altogether). > > > > I tried to correct that above but, if you prefer, the following would > > be closer to your phrasing but more correct/accurate: > > > > The @code{general_regs_only} function attribute informs > > the compiler that the function uses only general purpose > > registers. > > > > I don't understand what the second sentence is trying to say, and > > without a better error message for the problem in r99744, I suspect > > few users will either. I am suggesting to explain in the text you > > are adding, under what conditions inlining might fail without > > the attribute, and what effect the attribute has on the function > > that prevents the inlining failure. > > How about this? > > @item general_regs_only > @cindex @code{general_regs_only} function attribute, x86 > The @code{general_regs_only} function attribute informs the compiler > that the function uses only general purpose registers. When the > compiler inlines a function with the @code{always_inline} attribute, > target-specific compilation options may lead to inline failures. > The @code{general_regs_only} attribute, if applicable, can be used > together with the @code{always_inline} attribute to reduce inlining > failure. > > > (If we can't explain what the effect is then I wonder why > > the attribute is being added at all instead of teaching GCC to > > always behave as if the attribute were there when its absence > > would otherwise lead to an error.) > > > > Inlining an always_inline function into a function, which doesn't > support the ISA needed by the always_inline function, should fail. > But when inlining the always_inline function, the compiler doesn't > know if GPR instructions are sufficient for the always_inline function. > The general_regs_only informs the compiler that the function uses > only general purpose registers. There are no other ways for the > compiler to deduce such info at this stage. Is placing this attribute on a function definition which ends up using non-GPR regs invoking undefined behavior? That is, can IRA/LRA assume xmm regs need no saving around calls to such functions? Do we need to diagnose non-GPR uses (late, of course)? I'm not sure we're going to solve the always-inline issues with this new attribute - for example BMI intrinsics won't use non-GPR regs but then a function using those and marked as general-regs-only should still not be inlined into a function doing the BMI cpuid check. You document the attribute only affects always_inline decisions, this makes it even more special. I still think we should do the same for target attributes as we do for optimize attributes - always_inline should ignore any option differences and do what it is documented (inline): "@item always_inline @cindex @code{always_inline} function attribute Generally, functions are not inlined unless optimization is specified. For functions declared inline, this attribute inlines the function independent of any restrictions that otherwise apply to inlining. Failure to inline such a function is diagnosed as an error. Note that if such a function is called indirectly the compiler may or may not inline it depending on optimization level and a failure to inline an indirect call may or may not be diagnosed." nowhere does this suggest that inlining is not done if the caller is AVX2 but the callee is not. Richard. > -- > H.J.