From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0a-001b2d01.pphosted.com (mx0a-001b2d01.pphosted.com [148.163.156.1]) by sourceware.org (Postfix) with ESMTPS id 5B99C3858D29 for ; Fri, 17 Sep 2021 09:50:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 5B99C3858D29 Received: from pps.filterd (m0098409.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.1.2/8.16.1.2) with SMTP id 18H8U48H013078; Fri, 17 Sep 2021 05:50:57 -0400 Received: from pps.reinject (localhost [127.0.0.1]) by mx0a-001b2d01.pphosted.com with ESMTP id 3b4nc1575y-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 17 Sep 2021 05:50:56 -0400 Received: from m0098409.ppops.net (m0098409.ppops.net [127.0.0.1]) by pps.reinject (8.16.0.43/8.16.0.43) with SMTP id 18H8rCKY027478; Fri, 17 Sep 2021 05:50:56 -0400 Received: from ppma06fra.de.ibm.com (48.49.7a9f.ip4.static.sl-reverse.com [159.122.73.72]) by mx0a-001b2d01.pphosted.com with ESMTP id 3b4nc1575c-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 17 Sep 2021 05:50:56 -0400 Received: from pps.filterd (ppma06fra.de.ibm.com [127.0.0.1]) by ppma06fra.de.ibm.com (8.16.1.2/8.16.1.2) with SMTP id 18H9l6TC006526; Fri, 17 Sep 2021 09:50:53 GMT Received: from b06cxnps4075.portsmouth.uk.ibm.com (d06relay12.portsmouth.uk.ibm.com [9.149.109.197]) by ppma06fra.de.ibm.com with ESMTP id 3b0kqkjnh3-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 17 Sep 2021 09:50:53 +0000 Received: from d06av26.portsmouth.uk.ibm.com (d06av26.portsmouth.uk.ibm.com [9.149.105.62]) by b06cxnps4075.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 18H9opW852887962 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Fri, 17 Sep 2021 09:50:51 GMT Received: from d06av26.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 2D852AE05D; Fri, 17 Sep 2021 09:50:51 +0000 (GMT) Received: from d06av26.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id BC579AE05A; Fri, 17 Sep 2021 09:50:48 +0000 (GMT) Received: from kewenlins-mbp.cn.ibm.com (unknown [9.200.147.34]) by d06av26.portsmouth.uk.ibm.com (Postfix) with ESMTP; Fri, 17 Sep 2021 09:50:48 +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> <8a4da9c1-b46e-5176-2cde-65ac4a59dd75@linux.ibm.com> From: "Kewen.Lin" Message-ID: <8d230b16-e507-583f-7d98-d2ff45c7e656@linux.ibm.com> Date: Fri, 17 Sep 2021 17:50:47 +0800 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.10.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US X-TM-AS-GCONF: 00 X-Proofpoint-ORIG-GUID: 19DKDWxRiT-GMWymTjon_R5rxFGodAeT X-Proofpoint-GUID: GJh0NLy0_nJ7b9o0dMH5MMJLVZzd1ott Content-Transfer-Encoding: 8bit X-Proofpoint-UnRewURL: 0 URL was un-rewritten MIME-Version: 1.0 X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.182.1,Aquarius:18.0.790,Hydra:6.0.391,FMLib:17.0.607.475 definitions=2021-09-17_04,2021-09-16_01,2020-04-07_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 phishscore=0 spamscore=0 clxscore=1015 adultscore=0 malwarescore=0 mlxlogscore=999 lowpriorityscore=0 impostorscore=0 priorityscore=1501 bulkscore=0 suspectscore=0 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2109030001 definitions=main-2109170061 X-Spam-Status: No, score=-12.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, NICE_REPLY_A, RCVD_IN_MSPIKE_H3, 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: Fri, 17 Sep 2021 09:51:00 -0000 Hi Martin, on 2021/9/16 下午9:19, Martin Jambor wrote: > Hi, > > On Thu, Sep 16 2021, Kewen.Lin wrote: >> 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. > > All right. > Sorry that I failed to use 16 bit-fields for this, I figured out that the bit-fields can not be address-taken or passed as non-const reference. The gentype also failed to recognize uint16_t if I used uint16_t directly in ipa-fnsummary.h. Finally I used unsigned int instead. >>>> /* 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. > > 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 for confirming, I guess targets are very likely to have the need to scan the IFNs in future. I've moved it out of the block in V3. Thanks for noticing this! V3: https://gcc.gnu.org/pipermail/gcc-patches/2021-September/579658.html BR, Kewen