From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate.crashing.org (gate.crashing.org [63.228.1.57]) by sourceware.org (Postfix) with ESMTP id A393A385840B for ; Mon, 29 Nov 2021 16:58:14 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A393A385840B Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=kernel.crashing.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=kernel.crashing.org Received: from gate.crashing.org (localhost.localdomain [127.0.0.1]) by gate.crashing.org (8.14.1/8.14.1) with ESMTP id 1ATGvDCF031663; Mon, 29 Nov 2021 10:57:13 -0600 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 1ATGvCtX031662; Mon, 29 Nov 2021 10:57:12 -0600 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Mon, 29 Nov 2021 10:57:12 -0600 From: Segher Boessenkool To: "Kewen.Lin" Cc: GCC Patches , David Edelsohn , Bill Schmidt , Michael Meissner Subject: Re: [PATCH] rs6000: Fix some issues in rs6000_can_inline_p [PR102059] Message-ID: <20211129165712.GU614@gate.crashing.org> References: <3f2c6df2-e458-483c-facd-148a3cc3aead@linux.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <3f2c6df2-e458-483c-facd-148a3cc3aead@linux.ibm.com> User-Agent: Mutt/1.4.2.3i X-Spam-Status: No, score=-3.4 required=5.0 tests=BAYES_00, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=no autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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: Mon, 29 Nov 2021 16:58:16 -0000 Hi! On Wed, Sep 01, 2021 at 02:55:51PM +0800, Kewen.Lin wrote: > This patch is to fix the inconsistent behaviors for non-LTO mode > and LTO mode. As Martin pointed out, currently the function > rs6000_can_inline_p simply makes it inlinable if callee_tree is > NULL, but it's wrong, we should use the command line options > from target_option_default_node as default. This is not documented. > It also replaces > rs6000_isa_flags with the one from target_option_default_node > when caller_tree is NULL as rs6000_isa_flags could probably > change since initialization. Is this enough then? Are there no other places that use rs6000_isa_flags? Is it correct for us to have that variable at all anymore? Etc. > + bool always_inline = > + (DECL_DISREGARD_INLINE_LIMITS (callee) > + && lookup_attribute ("always_inline", DECL_ATTRIBUTES (callee))); The "=" should start a line, not end it. And please don't use unnecessary parens. > + /* Some flags such as fusion can be tolerated for always inlines. */ > + unsigned HOST_WIDE_INT always_inline_safe_mask = > + (MASK_P8_FUSION | MASK_P10_FUSION | OPTION_MASK_SAVE_TOC_INDIRECT > + | OPTION_MASK_P8_FUSION_SIGN | OPTION_MASK_P10_FUSION_LD_CMPI > + | OPTION_MASK_P10_FUSION_2LOGICAL | OPTION_MASK_P10_FUSION_LOGADD > + | OPTION_MASK_P10_FUSION_ADDLOG | OPTION_MASK_P10_FUSION_2ADD > + | OPTION_MASK_PCREL_OPT); Same. The fusion ones are obvious. The other two are not. Please put those two more obviously separate (not somewhere in the sea of fusion options), and some comment wouldn't hurt either. You can make it separate statements even, make it really stand out. Why are there OPTION_MASKs for separate P10 fusion types here, as well as MASK_P10_FUSION? > + > + if (always_inline) { > + caller_isa &= ~always_inline_safe_mask; > + callee_isa &= ~always_inline_safe_mask; > + } "{" starts a new line, indented. Segher