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 C2E243858D39; Wed, 29 Dec 2021 01:43:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C2E243858D39 Received: from pps.filterd (m0098396.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.1.2/8.16.1.2) with SMTP id 1BSNRvnN030329; Wed, 29 Dec 2021 01:43:36 GMT Received: from pps.reinject (localhost [127.0.0.1]) by mx0a-001b2d01.pphosted.com with ESMTP id 3d8121xpa1-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 29 Dec 2021 01:43:36 +0000 Received: from m0098396.ppops.net (m0098396.ppops.net [127.0.0.1]) by pps.reinject (8.16.0.43/8.16.0.43) with SMTP id 1BT1dWTq006608; Wed, 29 Dec 2021 01:43:35 GMT Received: from ppma06ams.nl.ibm.com (66.31.33a9.ip4.static.sl-reverse.com [169.51.49.102]) by mx0a-001b2d01.pphosted.com with ESMTP id 3d8121xp9r-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 29 Dec 2021 01:43:35 +0000 Received: from pps.filterd (ppma06ams.nl.ibm.com [127.0.0.1]) by ppma06ams.nl.ibm.com (8.16.1.2/8.16.1.2) with SMTP id 1BT1gfU5009787; Wed, 29 Dec 2021 01:43:33 GMT Received: from b06cxnps3074.portsmouth.uk.ibm.com (d06relay09.portsmouth.uk.ibm.com [9.149.109.194]) by ppma06ams.nl.ibm.com with ESMTP id 3d5tjjke5y-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 29 Dec 2021 01:43:33 +0000 Received: from d06av26.portsmouth.uk.ibm.com (d06av26.portsmouth.uk.ibm.com [9.149.105.62]) by b06cxnps3074.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 1BT1hUQh47382994 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 29 Dec 2021 01:43:30 GMT Received: from d06av26.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id D1662AE053; Wed, 29 Dec 2021 01:43:30 +0000 (GMT) Received: from d06av26.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 274CFAE045; Wed, 29 Dec 2021 01:43:29 +0000 (GMT) Received: from [9.200.154.17] (unknown [9.200.154.17]) by d06av26.portsmouth.uk.ibm.com (Postfix) with ESMTPS; Wed, 29 Dec 2021 01:43:28 +0000 (GMT) Message-ID: Date: Wed, 29 Dec 2021 09:43:26 +0800 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.4.1 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: wschmidt@linux.ibm.com, dje.gcc@gmail.com, gcc-patches@gcc.gnu.org, linkw@gcc.gnu.org, segher@kernel.crashing.org 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> <425d02f5-761d-8ea4-eea6-0fdaa136fbd9@linux.ibm.com> From: Xionghu Luo In-Reply-To: <425d02f5-761d-8ea4-eea6-0fdaa136fbd9@linux.ibm.com> Content-Type: text/plain; charset=UTF-8 X-TM-AS-GCONF: 00 X-Proofpoint-ORIG-GUID: TqTQKeDg4wVwPXIdODYDFrOGZnJS-QWt X-Proofpoint-GUID: GxVD-y_9LhXHQu-1clBPdfNXA3NQcJMc 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-28_13,2021-12-28_01,2021-12-02_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 malwarescore=0 phishscore=0 adultscore=0 impostorscore=0 lowpriorityscore=0 mlxscore=0 spamscore=0 bulkscore=0 priorityscore=1501 clxscore=1015 mlxlogscore=999 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2110150000 definitions=main-2112290005 X-Spam-Status: No, score=-13.7 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: Wed, 29 Dec 2021 01:43:41 -0000 On 2021/12/17 09:30, Xionghu Luo via Gcc-patches wrote: > > > 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 This is the last patch for the "cold bb in hot loop optimization" I'd like commit soon if not other comments, to let it fully tested more broadly before stage4. Thanks. Regression tested pass on powerpc64le-linux-gnu {P10,P9}, powerpc64-linux-gnu {P8, P7} and X86 though. > > 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" } } */ -- Thanks, Xionghu