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 DC35F3858001 for ; Thu, 2 Sep 2021 17:46:00 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org DC35F3858001 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=kernel.crashing.org Authentication-Results: sourceware.org; spf=fail 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 182Hiw5A028801; Thu, 2 Sep 2021 12:44:59 -0500 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 182HivTq028791; Thu, 2 Sep 2021 12:44:57 -0500 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Thu, 2 Sep 2021 12:44:56 -0500 From: Segher Boessenkool To: "Kewen.Lin" Cc: GCC Patches , Richard Biener , Jan Hubicka , Martin =?utf-8?B?TGnFoWth?= , Bill Schmidt , fweimer@redhat.com, mjambor@suse.cz Subject: Re: [RFC/PATCH] ipa-inline: Add target info into fn summary [PR102059] Message-ID: <20210902174456.GJ1583@gate.crashing.org> References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.4.2.3i X-Spam-Status: No, score=-5.6 required=5.0 tests=BAYES_00, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, TXREP, T_SPF_HELO_PERMERROR, T_SPF_PERMERROR 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: Thu, 02 Sep 2021 17:46:02 -0000 Hi! On Wed, Sep 01, 2021 at 03:02:22PM +0800, Kewen.Lin wrote: > It introduces two target hooks need_ipa_fn_target_info and > update_ipa_fn_target_info. The former allows target to do > some previous check and decides to collect target specific > information for this function or not. For some special case, > it can predict the analysis result and push it early without > any scannings. The latter allows the analyze_function_body > to pass gimple stmts down just like fp_expressions handlings, > target can do its own tricks. > > To make it simple, this patch uses HOST_WIDE_INT to record the > flags just like what we use for isa_flags. For rs6000's HTM > need, one HOST_WIDE_INT variable is quite enough, but it seems > good to have one auto_vec for scalability as I noticed some > targets have more than one HOST_WIDE_INT flag. For now, this > target information collection is only for always_inline function, > function ipa_merge_fn_summary_after_inlining deals with target > information merging. These flags can in principle be separate from any flags the target keeps, so 64 bits will be enough for a long time. If we want to architect that better, we should really architect the way all targets do target flags first. Let's not go there now :-) So just one HOST_WIDE_INT, not a stack of them please? > --- a/gcc/config/rs6000/rs6000-call.c > +++ b/gcc/config/rs6000/rs6000-call.c > @@ -13642,6 +13642,17 @@ rs6000_builtin_decl (unsigned code, bool initialize_p ATTRIBUTE_UNUSED) > return rs6000_builtin_decls[code]; > } > > +/* Return true if the builtin with CODE has any mask bits set > + which are specified by MASK. */ > + > +bool > +rs6000_builtin_mask_set_p (unsigned code, HOST_WIDE_INT mask) > +{ > + gcc_assert (code < RS6000_BUILTIN_COUNT); > + HOST_WIDE_INT fnmask = rs6000_builtin_info[code].mask; > + return fnmask & mask; > +} The "_p" does not say that "any bits" part, which is crucial here. So name this something like "rs6000_fn_has_any_of_these_mask_bits"? Yes the name sucks, because this interface does :-P Its it useful to have "any" semantics at all? Otherwise, require this to be passed just a single bit? The implicit "!!" (or "!= 0", same thing) that casting to bool does might be better explicit, too? A cast to bool changes value so is more suprising than other casts. > + /* Assume inline asm can use any instruction features. */ > + if (gimple_code (stmt) == GIMPLE_ASM) > + { > + info[0] = -1; > + return false; > + } What is -1 here? "All options set"? Does that work? Reliably? > + if (fndecl && fndecl_built_in_p (fndecl, BUILT_IN_MD)) > + { > + enum rs6000_builtins fcode = > + (enum rs6000_builtins) DECL_MD_FUNCTION_CODE (fndecl); > + /* HTM bifs definitely exploit HTM insns. */ > + if (rs6000_builtin_mask_set_p ((unsigned) fcode, RS6000_BTM_HTM)) Why the cast here? Please change the parameter type, instead? It is fine to use enums specific to our backend in that backend itself :-) > @@ -1146,6 +1147,16 @@ ipa_dump_fn_summary (FILE *f, struct cgraph_node *node) > fprintf (f, " calls:\n"); > dump_ipa_call_summary (f, 4, node, s); > fprintf (f, "\n"); > + HOST_WIDE_INT flags; > + for (int i = 0; s->target_info.iterate (i, &flags); i++) > + { > + if (i == 0) > + { > + fprintf (f, " target_info flags:"); > + } Don't use blocks around single statements please. > + /* Only look for target information for inlinable always_inline functions. */ > + bool scan_for_target_info = > + (info->inlinable > + && DECL_DISREGARD_INLINE_LIMITS (node->decl) > + && lookup_attribute ("always_inline", DECL_ATTRIBUTES (node->decl)) > + && targetm.target_option.need_ipa_fn_target_info (node->decl, > + info->target_info)); Don't use unnecessary parens please. Segher