From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0a-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com [148.163.158.5]) by sourceware.org (Postfix) with ESMTPS id 5D48C3858005 for ; Thu, 16 Sep 2021 03:44:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 5D48C3858005 Received: from pps.filterd (m0098413.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.1.2/8.16.0.43) with SMTP id 18G202N6006078; Wed, 15 Sep 2021 23:44:38 -0400 Received: from pps.reinject (localhost [127.0.0.1]) by mx0b-001b2d01.pphosted.com with ESMTP id 3b3vrnhe6f-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 15 Sep 2021 23:44:38 -0400 Received: from m0098413.ppops.net (m0098413.ppops.net [127.0.0.1]) by pps.reinject (8.16.0.43/8.16.0.43) with SMTP id 18G3eYYU024427; Wed, 15 Sep 2021 23:44:38 -0400 Received: from ppma04ams.nl.ibm.com (63.31.33a9.ip4.static.sl-reverse.com [169.51.49.99]) by mx0b-001b2d01.pphosted.com with ESMTP id 3b3vrnhe69-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 15 Sep 2021 23:44:37 -0400 Received: from pps.filterd (ppma04ams.nl.ibm.com [127.0.0.1]) by ppma04ams.nl.ibm.com (8.16.1.2/8.16.1.2) with SMTP id 18G3hIiA030023; Thu, 16 Sep 2021 03:44:36 GMT Received: from b06avi18626390.portsmouth.uk.ibm.com (b06avi18626390.portsmouth.uk.ibm.com [9.149.26.192]) by ppma04ams.nl.ibm.com with ESMTP id 3b0m3actqt-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 16 Sep 2021 03:44:35 +0000 Received: from b06wcsmtp001.portsmouth.uk.ibm.com (b06wcsmtp001.portsmouth.uk.ibm.com [9.149.105.160]) by b06avi18626390.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 18G3e1pL48759048 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 16 Sep 2021 03:40:01 GMT Received: from b06wcsmtp001.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 89444A4067; Thu, 16 Sep 2021 03:44:33 +0000 (GMT) Received: from b06wcsmtp001.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 823E0A405F; Thu, 16 Sep 2021 03:44:30 +0000 (GMT) Received: from KewenLins-MacBook-Pro.local (unknown [9.197.239.230]) by b06wcsmtp001.portsmouth.uk.ibm.com (Postfix) with ESMTP; Thu, 16 Sep 2021 03:44:30 +0000 (GMT) Subject: Re: [PATCH v2] ipa-inline: Add target info into fn summary [PR102059] To: Martin Jambor Cc: Richard Biener , Jan Hubicka , =?UTF-8?Q?Martin_Li=c5=a1ka?= , Segher Boessenkool , Bill Schmidt , fweimer@redhat.com, GCC Patches References: <0d10e5a2-a966-3b26-2e59-b6fd98d703a2@linux.ibm.com> From: "Kewen.Lin" Message-ID: <8a4da9c1-b46e-5176-2cde-65ac4a59dd75@linux.ibm.com> Date: Thu, 16 Sep 2021 11:44:28 +0800 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.10.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=gbk Content-Language: en-US Content-Transfer-Encoding: 8bit X-TM-AS-GCONF: 00 X-Proofpoint-ORIG-GUID: 3QvnHCl4_pqEHFDS6r3wGoAojIOAmfGb X-Proofpoint-GUID: QL5g7ZeEuh4Aof6L7QOnwXGj4BM29bJ- X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.182.1,Aquarius:18.0.687,Hydra:6.0.235,FMLib:17.0.607.475 definitions=2020-10-13_15,2020-10-13_02,2020-04-07_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 priorityscore=1501 clxscore=1015 lowpriorityscore=0 suspectscore=0 mlxlogscore=999 spamscore=0 adultscore=0 mlxscore=0 phishscore=0 impostorscore=0 bulkscore=0 malwarescore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2109030001 definitions=main-2109160008 X-Spam-Status: No, score=-10.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_EF, GIT_PATCH_0, MIME_CHARSET_FARAWAY, NICE_REPLY_A, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, 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 03:44:40 -0000 Hi Martin, Thanks for the review comments! on 2021/9/15 ÏÂÎç8:51, Martin Jambor wrote: > Hi, > > since this is inlining-related, I would somewhat prefer Honza to have a > look too, but I have the following comments: > > On Wed, Sep 08 2021, Kewen.Lin wrote: >> > > [...] > >> 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 information, >> + such as some target specific isa flags. */ >> + auto_vec GTY((skip)) target_info; >> /* Estimated growth for inlining all copies of the function before start >> of small functions inlining. >> This value will get out of date as the callers are duplicated, but > > 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. > 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. > 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. > Good point, will add it in v3. > [...] > > >> 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 = NULL; >> info->size_time_table.release (); >> info->call_size_time_table.release (); >> + info->target_info.release(); >> >> /* When optimizing and analyzing for IPA inliner, initialize loop optimizer >> so we can produce proper inline hints. >> @@ -2659,6 +2669,12 @@ analyze_function_body (struct cgraph_node *node, bool early) >> bb_predicate, >> bb_predicate); >> >> + /* Only look for target information for inlinable functions. */ >> + bool scan_for_target_info = >> + 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 = ENTRY_BLOCK_PTR_FOR_FN (cfun)->count; >> @@ -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 = >> + targetm.target_option.update_ipa_fn_target_info >> + (info->target_info, stmt); >> } > > 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. > > All that said, the overall approach seems correct to me. > Thanks again. BR, Kewen