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 EA7A23858D35 for ; Thu, 4 Nov 2021 10:56:31 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org EA7A23858D35 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 1A4AlO8h004589; Thu, 4 Nov 2021 10:56:30 GMT Received: from ppma02fra.de.ibm.com (47.49.7a9f.ip4.static.sl-reverse.com [159.122.73.71]) by mx0a-001b2d01.pphosted.com with ESMTP id 3c4e2x047m-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 04 Nov 2021 10:56:30 +0000 Received: from pps.filterd (ppma02fra.de.ibm.com [127.0.0.1]) by ppma02fra.de.ibm.com (8.16.1.2/8.16.1.2) with SMTP id 1A4Aq0Gd004671; Thu, 4 Nov 2021 10:56:27 GMT Received: from b06avi18626390.portsmouth.uk.ibm.com (b06avi18626390.portsmouth.uk.ibm.com [9.149.26.192]) by ppma02fra.de.ibm.com with ESMTP id 3c0wpac0pb-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 04 Nov 2021 10:56:27 +0000 Received: from d06av22.portsmouth.uk.ibm.com (d06av22.portsmouth.uk.ibm.com [9.149.105.58]) by b06avi18626390.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 1A4Anxcg47841568 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 4 Nov 2021 10:49:59 GMT Received: from d06av22.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 23AA34C04E; Thu, 4 Nov 2021 10:56:24 +0000 (GMT) Received: from d06av22.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 3DF894C044; Thu, 4 Nov 2021 10:56:22 +0000 (GMT) Received: from KewenLins-MacBook-Pro.local (unknown [9.200.47.106]) by d06av22.portsmouth.uk.ibm.com (Postfix) with ESMTP; Thu, 4 Nov 2021 10:56:21 +0000 (GMT) Subject: PING^6 [PATCH v2] combine: Tweak the condition of last_set invalidation To: GCC Patches Cc: Bill Schmidt , Segher Boessenkool References: <6bcd32fa-d0ef-b136-ddd9-92a1d21f60af@linux.ibm.com> <20210609201735.GJ18427@gate.crashing.org> <78d62c14-5731-17e3-e6a1-0deb4780ce37@linux.ibm.com> <8b439896-1d2a-a8a6-d044-97747a9535a2@linux.ibm.com> From: "Kewen.Lin" Message-ID: <6821cd5c-eb60-3c3a-915e-7eceb16954d1@linux.ibm.com> Date: Thu, 4 Nov 2021 18:56:20 +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=gbk Content-Language: en-US X-TM-AS-GCONF: 00 X-Proofpoint-GUID: xWs7pGomy73ZKnz3ZQfBSxrSsGWnmaOv X-Proofpoint-ORIG-GUID: xWs7pGomy73ZKnz3ZQfBSxrSsGWnmaOv 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.205,Aquarius:18.0.790,Hydra:6.0.425,FMLib:17.0.607.475 definitions=2021-11-04_03,2021-11-03_01,2020-04-07_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 mlxscore=0 mlxlogscore=999 lowpriorityscore=0 spamscore=0 bulkscore=0 malwarescore=0 impostorscore=0 suspectscore=0 priorityscore=1501 clxscore=1015 adultscore=0 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2110150000 definitions=main-2111040042 X-Spam-Status: No, score=-1.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_EF, KAM_SHORT, MIME_CHARSET_FARAWAY, RCVD_IN_MSPIKE_H2, 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, 04 Nov 2021 10:56:34 -0000 Hi, Gentle ping this: https://gcc.gnu.org/pipermail/gcc-patches/2021-June/572555.html BR, Kewen >>>>> on 2021/6/11 ÏÂÎç9:16, Kewen.Lin via Gcc-patches wrote: >>>>>> Hi Segher, >>>>>> >>>>>> Thanks for the review! >>>>>> >>>>>> on 2021/6/10 ÉÏÎç4:17, Segher Boessenkool wrote: >>>>>>> Hi! >>>>>>> >>>>>>> On Wed, Dec 16, 2020 at 04:49:49PM +0800, Kewen.Lin wrote: >>>>>>>> Currently we have the check: >>>>>>>> >>>>>>>> if (!insn >>>>>>>> || (value && rsp->last_set_table_tick >= label_tick_ebb_start)) >>>>>>>> rsp->last_set_invalid = 1; >>>>>>>> >>>>>>>> which means if we want to record some value for some reg and >>>>>>>> this reg got refered before in a valid scope, >>>>>>> >>>>>>> If we already know it is *set* in this same extended basic block. >>>>>>> Possibly by the same instruction btw. >>>>>>> >>>>>>>> we invalidate the >>>>>>>> set of reg (last_set_invalid to 1). It avoids to find the wrong >>>>>>>> set for one reg reference, such as the case like: >>>>>>>> >>>>>>>> ... op regX // this regX could find wrong last_set below >>>>>>>> regX = ... // if we think this set is valid >>>>>>>> ... op regX >>>>>>> >>>>>>> Yup, exactly. >>>>>>> >>>>>>>> But because of retry's existence, the last_set_table_tick could >>>>>>>> be set by some later reference insns, but we see it's set due >>>>>>>> to retry on the set (for that reg) insn again, such as: >>>>>>>> >>>>>>>> insn 1 >>>>>>>> insn 2 >>>>>>>> >>>>>>>> regX = ... --> (a) >>>>>>>> ... op regX --> (b) >>>>>>>> >>>>>>>> insn 3 >>>>>>>> >>>>>>>> // assume all in the same BB. >>>>>>>> >>>>>>>> Assuming we combine 1, 2 -> 3 sucessfully and replace them as two >>>>>>>> (3 insns -> 2 insns), >>>>>>> >>>>>>> This will delete insn 1 and write the combined result to insns 2 and 3. >>>>>>> >>>>>>>> retrying from insn1 or insn2 again: >>>>>>> >>>>>>> Always 2, but your point remains valid. >>>>>>> >>>>>>>> it will scan insn (a) again, the below condition holds for regX: >>>>>>>> >>>>>>>> (value && rsp->last_set_table_tick >= label_tick_ebb_start) >>>>>>>> >>>>>>>> it will mark this set as invalid set. But actually the >>>>>>>> last_set_table_tick here is set by insn (b) before retrying, so it >>>>>>>> should be safe to be taken as valid set. >>>>>>> >>>>>>> Yup. >>>>>>> >>>>>>>> This proposal is to check whether the last_set_table safely happens >>>>>>>> after the current set, make the set still valid if so. >>>>>>> >>>>>>>> Full SPEC2017 building shows this patch gets more sucessful combines >>>>>>>> from 1902208 to 1902243 (trivial though). >>>>>>> >>>>>>> Do you have some example, or maybe even a testcase? :-) >>>>>>> >>>>>> >>>>>> Sorry for the late reply, it took some time to get one reduced case. >>>>>> >>>>>> typedef struct SA *pa_t; >>>>>> >>>>>> struct SC { >>>>>> int h; >>>>>> pa_t elem[]; >>>>>> }; >>>>>> >>>>>> struct SD { >>>>>> struct SC *e; >>>>>> }; >>>>>> >>>>>> struct SA { >>>>>> struct { >>>>>> struct SD f[1]; >>>>>> } g; >>>>>> }; >>>>>> >>>>>> void foo(pa_t *k, char **m) { >>>>>> int l, i; >>>>>> pa_t a; >>>>>> l = (int)a->g.f[5].e; >>>>>> i = 0; >>>>>> for (; i < l; i++) { >>>>>> k[i] = a->g.f[5].e->elem[i]; >>>>>> m[i] = ""; >>>>>> } >>>>>> } >>>>>> >>>>>> Baseline is r12-0 and the option is "-O3 -mcpu=power9 -fno-strict-aliasing", >>>>>> with this patch, the generated assembly can save two rlwinm s. >>>>>> >>>>>>>> + /* Record the luid of the insn whose expression involving register n. */ >>>>>>>> + >>>>>>>> + int last_set_table_luid; >>>>>>> >>>>>>> "Record the luid of the insn for which last_set_table_tick was set", >>>>>>> right? >>>>>>> >>>>>> >>>>>> But it can be updated later to one smaller luid, how about the wording like: >>>>>> >>>>>> >>>>>> + /* Record the luid of the insn which uses register n, the insn should >>>>>> + be the first one using register n in that block of the insn which >>>>>> + last_set_table_tick was set for. */ >>>>>> >>>>>> >>>>>>>> -static void update_table_tick (rtx); >>>>>>>> +static void update_table_tick (rtx, int); >>>>>>> >>>>>>> Please remove this declaration instead, the function is not used until >>>>>>> after its actual definition :-) >>>>>>> >>>>>> >>>>>> Done. >>>>>> >>>>>>>> @@ -13243,7 +13247,21 @@ update_table_tick (rtx x) >>>>>>>> for (r = regno; r < endregno; r++) >>>>>>>> { >>>>>>>> reg_stat_type *rsp = ®_stat[r]; >>>>>>>> - rsp->last_set_table_tick = label_tick; >>>>>>>> + if (rsp->last_set_table_tick >= label_tick_ebb_start) >>>>>>>> + { >>>>>>>> + /* Later references should not have lower ticks. */ >>>>>>>> + gcc_assert (label_tick >= rsp->last_set_table_tick); >>>>>>> >>>>>>> This should be obvious, but checking it won't hurt, okay. >>>>>>> >>>>>>>> + /* Should pick up the lowest luid if the references >>>>>>>> + are in the same block. */ >>>>>>>> + if (label_tick == rsp->last_set_table_tick >>>>>>>> + && rsp->last_set_table_luid > insn_luid) >>>>>>>> + rsp->last_set_table_luid = insn_luid; >>>>>>> >>>>>>> Why? Is it conservative for the check you will do later? Please spell >>>>>>> this out, it is crucial! >>>>>>> >>>>>> >>>>>> Since later the combinations involving this insn probably make the >>>>>> register be used in one insn sitting ahead (which has smaller luid than >>>>>> the one which was recorded before). Yes, it's very conservative, this >>>>>> ensure that we always use the luid of the insn which is the first insn >>>>>> using this register in the block. The last_set invalidation is going >>>>>> to catch the case like: >>>>>> >>>>>> ... regX // avoid the set used here ... >>>>>> regX = ... >>>>>> ... >>>>>> >>>>>> Once we have the smallest luid one of all insns which use register X, >>>>>> any unsafe regX sets should be caught. >>>>>> >>>>>> I updated the comments to: >>>>>> >>>>>> + /* Since combination may generate some instructions >>>>>> + to replace some foregoing instructions with the >>>>>> + references to register r (using register r), we >>>>>> + need to make sure we record the first instruction >>>>>> + which is using register r, so always update with >>>>>> + the lowest luid here. If the given set happens >>>>>> + before this recorded earliest reference, the set >>>>>> + value should be safe to be used. */ >>>>>> >>>>>>>> @@ -13359,7 +13378,10 @@ record_value_for_reg (rtx reg, rtx_insn *insn, rtx value) >>>>>>>> >>>>>>>> /* Mark registers that are being referenced in this value. */ >>>>>>>> if (value) >>>>>>>> - update_table_tick (value); >>>>>>>> + { >>>>>>>> + gcc_assert (insn); >>>>>>>> + update_table_tick (value, DF_INSN_LUID (insn)); >>>>>>>> + } >>>>>>> >>>>>>> Don't add that assert please. If you really want one it should come >>>>>>> right at the start of the function, not 60 lines later :-) >>>>>>> >>>>>> >>>>>> Exactly, fixed. >>>>>> >>>>>>> Looks good if I understood this correctly :-) >>>>>>> >>>>>>> >>>>>> >>>>>> Thanks again, I also updated the comments in func record_value_for_reg, >>>>>> the new version is attached. >>>>>> >>>>>> BR, >>>>>> Kewen >>>>>> ----- >>>>>> gcc/ChangeLog: >>>>>> >>>>>> * combine.c (struct reg_stat_type): New member >>>>>> last_set_table_luid. >>>>>> (update_table_tick): Add one argument for insn luid and >>>>>> set last_set_table_luid with it, remove its declaration. >>>>>> (record_value_for_reg): Adjust the condition to set >>>>>> last_set_invalid nonzero. >>>>>>