From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.220.29]) by sourceware.org (Postfix) with ESMTPS id 3BF2B3858C39 for ; Thu, 16 Sep 2021 13:19:12 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 3BF2B3858C39 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=suse.cz Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.cz Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id 116311FEE1; Thu, 16 Sep 2021 13:19:11 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_rsa; t=1631798351; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=AANXgvQjKOEpUCgn07Z7+HQtQtuhjs0gHyoUO45LK0I=; b=xdEr/6iK9iRe/jz7lOIj/Ow3EHB6NMIc1Anq1IcSgZ5VJGd9oG0U0OGIZb3lgUbBDAshgz s22aXWnrO+do975B9PYiXhmvevlklhEe9pOkbtWBBk6iaCxVUKA+VTj5FIKuMXsSQznsv1 vVZVsubCIXv/Vz8Xo29BJcVzkVUJ66s= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_ed25519; t=1631798351; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=AANXgvQjKOEpUCgn07Z7+HQtQtuhjs0gHyoUO45LK0I=; b=BD4kVVvqmq+0NyrBIOhquLjf3ELfx7JlXWZWspdF0MVUTbxZjOB0iaQrnN2crhdHiuCcFh ormXY743snu3mtAA== Received: from suse.cz (virgil.suse.cz [10.100.13.50]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id DEA44A3B90; Thu, 16 Sep 2021 13:19:10 +0000 (UTC) From: Martin Jambor To: "Kewen.Lin" Cc: Richard Biener , Jan Hubicka , Martin =?utf-8?Q?Li=C5=A1ka?= , Segher Boessenkool , Bill Schmidt , fweimer@redhat.com, GCC Patches Subject: Re: [PATCH v2] ipa-inline: Add target info into fn summary [PR102059] In-Reply-To: <8a4da9c1-b46e-5176-2cde-65ac4a59dd75@linux.ibm.com> References: <0d10e5a2-a966-3b26-2e59-b6fd98d703a2@linux.ibm.com> <8a4da9c1-b46e-5176-2cde-65ac4a59dd75@linux.ibm.com> User-Agent: Notmuch/0.32.3 (https://notmuchmail.org) Emacs/27.2 (x86_64-suse-linux-gnu) Date: Thu, 16 Sep 2021 15:19:10 +0200 Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-11.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham 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, 16 Sep 2021 13:19:14 -0000 Hi, On Thu, Sep 16 2021, Kewen.Lin wrote: > Hi Martin, > > Thanks for the review comments! > > on 2021/9/15 =E4=B8=8B=E5=8D=888:51, Martin Jambor wrote: >> Hi, >>=20 >> since this is inlining-related, I would somewhat prefer Honza to have a >> look too, but I have the following comments: >>=20 >> On Wed, Sep 08 2021, Kewen.Lin wrote: >>> >>=20 >> [...] >>=20 >>> diff --git a/gcc/ipa-fnsummary.h b/gcc/ipa-fnsummary.h >>> index 78399b0b9bb..300b8da4507 100644 >>> --- a/gcc/ipa-fnsummary.h >>> +++ b/gcc/ipa-fnsummary.h >>> @@ -193,6 +194,9 @@ public: >>> vec *loop_strides; >>> /* Parameters tested by builtin_constant_p. */ >>> vec GTY((skip)) builtin_constant_p_parms; >>> + /* Like fp_expressions, but it's to hold some target specific inform= ation, >>> + such as some target specific isa flags. */ >>> + auto_vec GTY((skip)) target_info; >>> /* Estimated growth for inlining all copies of the function before s= tart >>> of small functions inlining. >>> This value will get out of date as the callers are duplicated, but >>=20 >> Segher already wrote in the first thread that a vector of HOST_WIDE_INTs >> is an overkill and I agree. So at least make the new field just a >> HOST_WIDE_INT or better yet, an unsigned int. But I would even go >> further and make target_info only a 16-bit bit-field, place it after the >> other bit-fields in class ipa_fn_summary and pass it to the hooks as >> uint16_t. Unless you have plans which require more space, I think we >> should be conservative here. >>=20 > > OK, yeah, the consideration is mainly for the scenario that target has > a few bits to care about. I just realized that to avoid inefficient > bitwise operation for mapping target info bits to isa_flag bits, target > can rearrange the sparse bits in isa_flag, so it's not a deal. > Thanks for re-raising this! I'll use the 16 bits bit-field in v3 as you > suggested, if you don't mind, I will put it before the existing bit-fields > to have a good alignment. All right. > >> I am also not sure if I agree that the field should not be streamed for >> offloading, but since we do not have an offloading compiler needing them >> I guess for now that is OK. But it should be documented in the comment >> describing the field that it is not streamed to offloading compilers. >>=20 > > Good point, will add it in v3. > >> [...] >>=20 >>=20 >>> diff --git a/gcc/ipa-fnsummary.c b/gcc/ipa-fnsummary.c >>> index 2470937460f..72091b6193f 100644 >>> --- a/gcc/ipa-fnsummary.c >>> +++ b/gcc/ipa-fnsummary.c >>> @@ -2608,6 +2617,7 @@ analyze_function_body (struct cgraph_node *node, = bool early) >>> info->conds =3D NULL; >>> info->size_time_table.release (); >>> info->call_size_time_table.release (); >>> + info->target_info.release(); >>>=20=20 >>> /* When optimizing and analyzing for IPA inliner, initialize loop op= timizer >>> so we can produce proper inline hints. >>> @@ -2659,6 +2669,12 @@ analyze_function_body (struct cgraph_node *node,= bool early) >>> bb_predicate, >>> bb_predicate); >>>=20=20 >>> + /* Only look for target information for inlinable functions. */ >>> + bool scan_for_target_info =3D >>> + info->inlinable >>> + && targetm.target_option.need_ipa_fn_target_info (node->decl, >>> + info->target_info); >>> + >>> if (fbi.info) >>> compute_bb_predicates (&fbi, node, info, params_summary); >>> const profile_count entry_count =3D ENTRY_BLOCK_PTR_FOR_FN (cfun)->c= ount; >>> @@ -2876,6 +2892,10 @@ analyze_function_body (struct cgraph_node *node,= bool early) >>> if (dump_file) >>> fprintf (dump_file, " fp_expression set\n"); >>> } >>> + if (scan_for_target_info) >>> + scan_for_target_info =3D >>> + targetm.target_option.update_ipa_fn_target_info >>> + (info->target_info, stmt); >>> } >>=20 >> Practically it probably does not matter, but why is this in the "if >> (this_time || this_size)" block? Although I can see that setting >> fp_expression is also done that way... but it seems like copying a >> mistake to me. > > Yeah, I felt target info scanning is similar to fp_expression scanning, > so I just followed the same way. If I read it right, the case > !(this_time || this_size) means the STMT won't be weighted to any RTL > insn from both time and size perspectives, so guarding it seems to avoid > unnecessary scannings. I assumed that target bifs and inline asm would > not be evaluated as zero cost, it seems safe so far for HTM usage. > > Do you worry about some special STMT which is weighted to zero but it's > necessarily to be checked for target info in a long term? > If so, I'll move it out in v3. It seems that gimple_call_internal_p statements are always costed to zero and I am wondering whether those are something that targets would want to look out for in the future. But hopefully anyone implementing that in the future would come up with a testcase and would need to fix this to have the testcase pass. Thanks, Martin >>=20 >> All that said, the overall approach seems correct to me. >>=20 > > Thanks again. > BR, > Kewen