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 C42B93858D28; Fri, 17 Dec 2021 01:30:33 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C42B93858D28 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 1BGMlQV6018786; Fri, 17 Dec 2021 01:30:32 GMT Received: from pps.reinject (localhost [127.0.0.1]) by mx0a-001b2d01.pphosted.com with ESMTP id 3cyn1kc2g9-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 17 Dec 2021 01:30:32 +0000 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 1BH1UVRK008907; Fri, 17 Dec 2021 01:30:31 GMT Received: from ppma04fra.de.ibm.com (6a.4a.5195.ip4.static.sl-reverse.com [149.81.74.106]) by mx0a-001b2d01.pphosted.com with ESMTP id 3cyn1kc2fp-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 17 Dec 2021 01:30:31 +0000 Received: from pps.filterd (ppma04fra.de.ibm.com [127.0.0.1]) by ppma04fra.de.ibm.com (8.16.1.2/8.16.1.2) with SMTP id 1BH1Cvup028514; Fri, 17 Dec 2021 01:30:29 GMT Received: from b06cxnps4075.portsmouth.uk.ibm.com (d06relay12.portsmouth.uk.ibm.com [9.149.109.197]) by ppma04fra.de.ibm.com with ESMTP id 3cy7vvujvj-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 17 Dec 2021 01:30:28 +0000 Received: from d06av21.portsmouth.uk.ibm.com (d06av21.portsmouth.uk.ibm.com [9.149.105.232]) by b06cxnps4075.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 1BH1UQrJ35127614 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Fri, 17 Dec 2021 01:30:26 GMT Received: from d06av21.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 3FD5552059; Fri, 17 Dec 2021 01:30:26 +0000 (GMT) Received: from [9.197.241.46] (unknown [9.197.241.46]) by d06av21.portsmouth.uk.ibm.com (Postfix) with ESMTPS id 0DA245204E; Fri, 17 Dec 2021 01:30:23 +0000 (GMT) Message-ID: <425d02f5-761d-8ea4-eea6-0fdaa136fbd9@linux.ibm.com> Date: Fri, 17 Dec 2021 09:30:21 +0800 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.4.0 Subject: Re: [PATCH 1/3] loop-invariant: Don't move cold bb instructions to preheader in RTL Content-Language: en-US To: Jan Hubicka Cc: gcc-patches@gcc.gnu.org, segher@kernel.crashing.org, wschmidt@linux.ibm.com, linkw@gcc.gnu.org, dje.gcc@gmail.com References: <20211208055416.1415283-1-luoxhu@linux.ibm.com> <20211208055416.1415283-2-luoxhu@linux.ibm.com> <20211213091441.GC79706@kam.mff.cuni.cz> <20211213102450.GK50931@kam.mff.cuni.cz> <20211216112009.GF4516@kam.mff.cuni.cz> From: Xionghu Luo In-Reply-To: <20211216112009.GF4516@kam.mff.cuni.cz> Content-Type: text/plain; charset=UTF-8 X-TM-AS-GCONF: 00 X-Proofpoint-ORIG-GUID: tSsxWs9j7EW3pwZGletNL_-5_9o1xDi9 X-Proofpoint-GUID: JPqBVR4sMfOxRMXX6WanA6odvj39-5rw Content-Transfer-Encoding: 7bit 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.11.62.513 definitions=2021-12-16_09,2021-12-16_01,2021-12-02_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 phishscore=0 mlxlogscore=999 lowpriorityscore=0 priorityscore=1501 impostorscore=0 suspectscore=0 clxscore=1015 malwarescore=0 spamscore=0 mlxscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2110150000 definitions=main-2112170005 X-Spam-Status: No, score=-12.4 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, NICE_REPLY_A, 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: Fri, 17 Dec 2021 01:30:35 -0000 On 2021/12/16 19:20, Jan Hubicka wrote: >> >> OK. Comments like? >> >> /* Don't move insn of cold BB out of loop to preheader to reduce calculations >> and register live range in hot loop with cold BB. */ > > Looks good. >> >> >> And maybe some dump log will help tracking in xxx.c.271r.loop2_invariant. >> >> --- a/gcc/loop-invariant.c >> +++ b/gcc/loop-invariant.c >> @@ -1190,7 +1190,12 @@ find_invariants_bb (class loop *loop, basic_block bb, bool always_reached, >> basic_block preheader = loop_preheader_edge (loop)->src; >> >> if (preheader->count > bb->count) >> - return; >> + { >> + if (dump_file) >> + fprintf (dump_file, "Don't move invariant from bb: %d in loop %d\n", >> + bb->index, loop->num); >> + return; >> + } > > This is also a good idea - testcases are quite important for this typo > of stuff. >>> >>> Thinking about this more, you want to test: >>> if (!always_executed && preheader->count > bb->count) >>> return; >>> This will rule out some profile misupates. >>> >>> Also the code updating always_reached is worng: >>> if (always_reached >>> && CALL_P (insn) >>> && (RTL_LOOPING_CONST_OR_PURE_CALL_P (insn) >>> || ! RTL_CONST_OR_PURE_CALL_P (insn))) >>> always_reached = false; >>> It misses the case where statement can trhow external exception or >>> volatile asms that can also terminate execution. >>> >>> I bit worry on how often the test will have false positives with guessed >>> profile where earlier loop optimizations reduced trip count below >>> realistic estimate. >> >> Sorry I don't understand why always_executed and always_reached matters here? >> the test is based on BB before the FOR_BB_INSNS loop... > > always_executed is useful here to avoid the situation where bb->count or > preheader->count is wrong and it would disable wanted transformation. > > always_executed is just a bug I noticed while looking at the code. I > will produce testcase for bugzilla. > > Honza Thanks, so is this OK to commit now? And any additional comments for the "[PATCH 3/3] Fix loop split incorrect count and probability" (https://gcc.gnu.org/pipermail/gcc-patches/2021-December/586374.html)? Updated comments and testcase: [PATCH 1/3] loop-invariant: Don't move cold bb instructions to preheader in RTL From: Xiong Hu Luo gcc/ChangeLog: * loop-invariant.c (find_invariants_bb): Check profile count before motion. (find_invariants_body): Add argument. gcc/testsuite/ChangeLog: * gcc.dg/loop-invariant-2.c: New. --- gcc/loop-invariant.c | 17 ++++++++++++++--- gcc/testsuite/gcc.dg/loop-invariant-2.c | 20 ++++++++++++++++++++ 2 files changed, 34 insertions(+), 3 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/loop-invariant-2.c diff --git a/gcc/loop-invariant.c b/gcc/loop-invariant.c index fca0c2b24be..690f7704a0b 100644 --- a/gcc/loop-invariant.c +++ b/gcc/loop-invariant.c @@ -1183,9 +1183,21 @@ find_invariants_insn (rtx_insn *insn, bool always_reached, bool always_executed) call. */ static void -find_invariants_bb (basic_block bb, bool always_reached, bool always_executed) +find_invariants_bb (class loop *loop, basic_block bb, bool always_reached, + bool always_executed) { rtx_insn *insn; + basic_block preheader = loop_preheader_edge (loop)->src; + + /* Don't move insn of cold BB out of loop to preheader to reduce calculations + and register live range in hot loop with cold BB. */ + if (!always_executed && preheader->count > bb->count) + { + if (dump_file) + fprintf (dump_file, "Don't move invariant from bb: %d out of loop %d\n", + bb->index, loop->num); + return; + } FOR_BB_INSNS (bb, insn) { @@ -1214,8 +1226,7 @@ find_invariants_body (class loop *loop, basic_block *body, unsigned i; for (i = 0; i < loop->num_nodes; i++) - find_invariants_bb (body[i], - bitmap_bit_p (always_reached, i), + find_invariants_bb (loop, body[i], bitmap_bit_p (always_reached, i), bitmap_bit_p (always_executed, i)); } diff --git a/gcc/testsuite/gcc.dg/loop-invariant-2.c b/gcc/testsuite/gcc.dg/loop-invariant-2.c new file mode 100644 index 00000000000..df3d8458569 --- /dev/null +++ b/gcc/testsuite/gcc.dg/loop-invariant-2.c @@ -0,0 +1,20 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fdump-rtl-loop2_invariant" } */ + +volatile int x; +void +bar (int, char *, char *); +void +foo (int *a, int n, int k) +{ + int i; + + for (i = 0; i < n; i++) + { + if (__builtin_expect (x, 0)) + bar (k / 5, "one", "two"); + a[i] = k; + } +} + +/* { dg-final { scan-rtl-dump "Don't move invariant from bb: .*out of loop" "loop2_invariant" } } */ -- 2.27.0.90.geebb51ba8c