Message ID | eb7f49a5-7e58-4a02-e4d1-f2d0cc0061e1@linux.vnet.ibm.com |
---|---|
State | New, archived |
Headers |
Return-Path: <gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:ecc5:0:0:0:0:0 with SMTP id s5csp910304wro; Tue, 23 Aug 2022 04:50:31 -0700 (PDT) X-Google-Smtp-Source: AA6agR6jCFrG+bTBdz3Lc0VQCliWWuMrsZ7sGEPAPL9VmoIKF694yRfZydW79avC9jRwteWVOYn2 X-Received: by 2002:a05:6402:3711:b0:445:e264:beed with SMTP id ek17-20020a056402371100b00445e264beedmr3438387edb.136.1661255430990; Tue, 23 Aug 2022 04:50:30 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1661255430; cv=none; d=google.com; s=arc-20160816; b=yryn1VCquZoVj8UCNaHCb28MmZRIfStV7YdoCC352tpmbl//2LlPHqAJPkHpM4j6JF ybrjtAFc8UdFVkqGppnxpR12/gmk6t5uOqTKXwO92VAAn6oGmsAc/psGlz7VS9Ldux1n sL8T8B3WqOMJThofS5kX/TW3GlAn1Gx/ETz6/u7Emndf2pz5hcF973Pj+NcjwdnvkhiF yU17eMQdYZ1eRDXFPg1Bdgcsuhwk9xYfFAaoyvFsCcgspdExtFeu7lR9YnAyemVwFRI1 w1SUNzLnoQesmvla2t9cgEuhKMY3z0pubfULe/+WwpXBSKAoe4o11WvdDQqRigeL97FA nPvg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=sender:errors-to:reply-to:from:list-subscribe:list-help:list-post :list-archive:list-unsubscribe:list-id:precedence :content-transfer-encoding:subject:to:content-language:user-agent :mime-version:date:message-id:dmarc-filter:delivered-to :dkim-signature:dkim-filter; bh=T5DnUsDTRq21mUGG7f8IFVkKCgXeCBWOCWwmOZ1PrrA=; b=GOhdlf2N5T5NA+x3pidosVdnmmnm4aRIakBeBkZ+VVm/xJE9ka+jcXeepeNY2HfuW5 93M2xVGQvT1tjHhbDhkx6rL2AXXsHEHodg3ufpvqh1LPxezZLTAC8BdbHnXMaLlUTWRU FNW6tWalxXDSBMLLK6A3o4k69eqA7FBn/QOFpEumXaEKlpecdYjU7tC458e5ZDYNRNkP bYLiYv3qqTHVBwiP2745dMA0ZGJ/gtVC+BMJOeU+sQvuLWJ1ROgtYl5vl2x4J72cIHCA qZqsVpxIf4P5A6zOXaO1t24klItPiab3nX9/sOay4msnArd5QuxxjSCW713wmBSyOpAR SJiQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gcc.gnu.org header.s=default header.b=N7npX0WM; spf=pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 2620:52:3:1:0:246e:9693:128c as permitted sender) smtp.mailfrom="gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=gnu.org Received: from sourceware.org (server2.sourceware.org. [2620:52:3:1:0:246e:9693:128c]) by mx.google.com with ESMTPS id cr2-20020a170906d54200b007341ad4b028si13528439ejc.642.2022.08.23.04.50.30 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 23 Aug 2022 04:50:30 -0700 (PDT) Received-SPF: pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 2620:52:3:1:0:246e:9693:128c as permitted sender) client-ip=2620:52:3:1:0:246e:9693:128c; Authentication-Results: mx.google.com; dkim=pass header.i=@gcc.gnu.org header.s=default header.b=N7npX0WM; spf=pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 2620:52:3:1:0:246e:9693:128c as permitted sender) smtp.mailfrom="gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=gnu.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id C5B30385828E for <ouuuleilei@gmail.com>; Tue, 23 Aug 2022 11:50:29 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org C5B30385828E DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1661255429; bh=T5DnUsDTRq21mUGG7f8IFVkKCgXeCBWOCWwmOZ1PrrA=; h=Date:To:Subject:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=N7npX0WMAIQvd8C4zCX/STaC1/+lrBw/xfXECSH7tKxe4F7wRhbPwxeWmvnT5a3hQ oAFhqdMCIX4KxPVk5/iaf60gE3aH+M+Nhq8hhzS0mIV+1iNeaqAOvYYnt6bZjAgC0q Ojmygb298hP4ck8r1OP7sMI+4HVCAHV2R3N2BgrM= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mx0a-001b2d01.pphosted.com (mx0a-001b2d01.pphosted.com [148.163.156.1]) by sourceware.org (Postfix) with ESMTPS id 9D12A3858D32 for <gcc-patches@gcc.gnu.org>; Tue, 23 Aug 2022 11:49:46 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 9D12A3858D32 Received: from pps.filterd (m0187473.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.17.1.5/8.17.1.5) with ESMTP id 27NBko4e013247; Tue, 23 Aug 2022 11:49:45 GMT Received: from ppma03dal.us.ibm.com (b.bd.3ea9.ip4.static.sl-reverse.com [169.62.189.11]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3j4xat829n-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 23 Aug 2022 11:49:44 +0000 Received: from pps.filterd (ppma03dal.us.ibm.com [127.0.0.1]) by ppma03dal.us.ibm.com (8.16.1.2/8.16.1.2) with SMTP id 27NBZrh7018877; Tue, 23 Aug 2022 11:49:44 GMT Received: from b01cxnp23033.gho.pok.ibm.com (b01cxnp23033.gho.pok.ibm.com [9.57.198.28]) by ppma03dal.us.ibm.com with ESMTP id 3j2q89q93s-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 23 Aug 2022 11:49:44 +0000 Received: from b01ledav006.gho.pok.ibm.com (b01ledav006.gho.pok.ibm.com [9.57.199.111]) by b01cxnp23033.gho.pok.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 27NBngX566847152 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 23 Aug 2022 11:49:42 GMT Received: from b01ledav006.gho.pok.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 35FD9AC05B; Tue, 23 Aug 2022 11:49:42 +0000 (GMT) Received: from b01ledav006.gho.pok.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id D9348AC059; Tue, 23 Aug 2022 11:49:40 +0000 (GMT) Received: from [9.43.70.41] (unknown [9.43.70.41]) by b01ledav006.gho.pok.ibm.com (Postfix) with ESMTP; Tue, 23 Aug 2022 11:49:40 +0000 (GMT) Message-ID: <eb7f49a5-7e58-4a02-e4d1-f2d0cc0061e1@linux.vnet.ibm.com> Date: Tue, 23 Aug 2022 17:19:39 +0530 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Content-Language: en-US To: gcc-patches@gcc.gnu.org, bergner@linux.ibm.com, Segher Boessenkool <segher@kernel.crashing.org>, pthaugen@us.ibm.com Subject: [PATCH] sched1: Fix -fcompare-debug issue in schedule_region [PR105586] Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 X-Proofpoint-ORIG-GUID: DljN6khxwH0gQxOy_nZwrBksZydhi_T6 X-Proofpoint-GUID: DljN6khxwH0gQxOy_nZwrBksZydhi_T6 X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.517,FMLib:17.11.122.1 definitions=2022-08-23_04,2022-08-22_02,2022-06-22_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 priorityscore=1501 mlxlogscore=999 impostorscore=0 mlxscore=0 adultscore=0 bulkscore=0 clxscore=1011 malwarescore=0 spamscore=0 phishscore=0 lowpriorityscore=0 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2207270000 definitions=main-2208230046 X-Spam-Status: No, score=-11.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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 <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> From: Surya Kumari Jangala via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: Surya Kumari Jangala <jskumari@linux.vnet.ibm.com> Errors-To: gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org> X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1741952574730418061?= X-GMAIL-MSGID: =?utf-8?q?1741952574730418061?= |
Series |
sched1: Fix -fcompare-debug issue in schedule_region [PR105586]
|
|
Commit Message
Surya Kumari Jangala
Aug. 23, 2022, 11:49 a.m. UTC
sched1: Fix -fcompare-debug issue in schedule_region [PR105586] In schedule_region(), a basic block that does not contain any real insns is not scheduled and the dfa state at the entry of the bb is not copied to the fallthru basic block. However a DEBUG insn is treated as a real insn, and if a bb contains non-real insns and a DEBUG insn, it's dfa state is copied to the fallthru bb. This was resulting in -fcompare-debug failure as the incoming dfa state of the fallthru block is different with -g. We should always copy the dfa state of a bb to it's fallthru bb even if the bb does not contain real insns. 2022-08-22 Surya Kumari Jangala <jskumari@linux.ibm.com> gcc/ PR rtl-optimization/105586 * sched-rgn.cc (schedule_region): Always copy dfa state to fallthru block. gcc/testsuite/ PR rtl-optimization/105586 * gcc.target/powerpc/pr105586.c: New test.
Comments
On 8/23/22 6:49 AM, Surya Kumari Jangala wrote: > sched1: Fix -fcompare-debug issue in schedule_region [PR105586] > > In schedule_region(), a basic block that does not contain any real insns > is not scheduled and the dfa state at the entry of the bb is not copied > to the fallthru basic block. However a DEBUG insn is treated as a real > insn, and if a bb contains non-real insns and a DEBUG insn, it's dfa > state is copied to the fallthru bb. This was resulting in > -fcompare-debug failure as the incoming dfa state of the fallthru block > is different with -g. We should always copy the dfa state of a bb to > it's fallthru bb even if the bb does not contain real insns. > > 2022-08-22 Surya Kumari Jangala <jskumari@linux.ibm.com> > > gcc/ > PR rtl-optimization/105586 > * sched-rgn.cc (schedule_region): Always copy dfa state to > fallthru block. It looks good to me, but I cannot approve it. That said, you're missing a ChangeLog entry for your new helper function. The ChangeLog mentions what changed, not why it changed. Maybe something like the following? gcc/ PR rtl-optimization/105586 * sched-rgn.cc (save_state_for_fallthru_edge): New function. (schedule_region): Use it for all blocks. Peter
Hi! On Tue, Aug 23, 2022 at 07:55:22AM -0500, Peter Bergner wrote: > It looks good to me, but I cannot approve it. Same here (both statements). > That said, you're missing > a ChangeLog entry for your new helper function. The ChangeLog mentions > what changed, not why it changed. And that is correct! Changelogs should not say that, that isn't their purpose (in GCC), not what they are used for. Explanations like that go in the email and/or the commit message. The main remaining usefulness of changelogs is to spot unintended commmits. > Maybe something like the following? > > gcc/ > PR rtl-optimization/105586 > * sched-rgn.cc (save_state_for_fallthru_edge): New function. > (schedule_region): Use it for all blocks. That looks perfect, it doesn't say "why" either :-) Segher
Hi Peter, Segher, Thanks for going thru the patch! I will make the proposed changes to the Changelog. Regards, Surya On 23/08/22 6:58 pm, Segher Boessenkool wrote: > Hi! > > On Tue, Aug 23, 2022 at 07:55:22AM -0500, Peter Bergner wrote: >> It looks good to me, but I cannot approve it. > > Same here (both statements). > >> That said, you're missing >> a ChangeLog entry for your new helper function. The ChangeLog mentions >> what changed, not why it changed. > > And that is correct! Changelogs should not say that, that isn't their > purpose (in GCC), not what they are used for. Explanations like that go > in the email and/or the commit message. > > The main remaining usefulness of changelogs is to spot unintended > commmits. > >> Maybe something like the following? >> >> gcc/ >> PR rtl-optimization/105586 >> * sched-rgn.cc (save_state_for_fallthru_edge): New function. >> (schedule_region): Use it for all blocks. > > That looks perfect, it doesn't say "why" either :-) > > > Segher
Surya Kumari Jangala via Gcc-patches <gcc-patches@gcc.gnu.org> writes: > sched1: Fix -fcompare-debug issue in schedule_region [PR105586] > > In schedule_region(), a basic block that does not contain any real insns > is not scheduled and the dfa state at the entry of the bb is not copied > to the fallthru basic block. However a DEBUG insn is treated as a real > insn, and if a bb contains non-real insns and a DEBUG insn, it's dfa > state is copied to the fallthru bb. This was resulting in > -fcompare-debug failure as the incoming dfa state of the fallthru block > is different with -g. We should always copy the dfa state of a bb to > it's fallthru bb even if the bb does not contain real insns. > > 2022-08-22 Surya Kumari Jangala <jskumari@linux.ibm.com> > > gcc/ > PR rtl-optimization/105586 > * sched-rgn.cc (schedule_region): Always copy dfa state to > fallthru block. > > gcc/testsuite/ > PR rtl-optimization/105586 > * gcc.target/powerpc/pr105586.c: New test. > > > diff --git a/gcc/sched-rgn.cc b/gcc/sched-rgn.cc > index 0dc2a8f2851..6c9202c0b2b 100644 > --- a/gcc/sched-rgn.cc > +++ b/gcc/sched-rgn.cc > @@ -3082,6 +3082,24 @@ free_bb_state_array (void) > bb_state = NULL; > } > > +static void > +save_state_for_fallthru_edge (basic_block last_bb, state_t state) The convention (not always followed) is to have a comment above each function explaining what it does. How about something like: /* If LAST_BB falls through to another block B, record that B should start with DFA start STATE. */ OK for trunk with that change (and with the new changelog). Thanks, Richard > +{ > + edge f = find_fallthru_edge (last_bb->succs); > + if (f > + && (!f->probability.initialized_p () > + || (f->probability.to_reg_br_prob_base () * 100 > + / REG_BR_PROB_BASE > + >= param_sched_state_edge_prob_cutoff))) > + { > + memcpy (bb_state[f->dest->index], state, > + dfa_state_size); > + if (sched_verbose >= 5) > + fprintf (sched_dump, "saving state for edge %d->%d\n", > + f->src->index, f->dest->index); > + } > +} > + > /* Schedule a region. A region is either an inner loop, a loop-free > subroutine, or a single basic block. Each bb in the region is > scheduled after its flow predecessors. */ > @@ -3155,6 +3173,7 @@ schedule_region (int rgn) > if (no_real_insns_p (head, tail)) > { > gcc_assert (first_bb == last_bb); > + save_state_for_fallthru_edge (last_bb, bb_state[first_bb->index]); > continue; > } > > @@ -3173,26 +3192,13 @@ schedule_region (int rgn) > curr_bb = first_bb; > if (dbg_cnt (sched_block)) > { > - edge f; > int saved_last_basic_block = last_basic_block_for_fn (cfun); > > schedule_block (&curr_bb, bb_state[first_bb->index]); > gcc_assert (EBB_FIRST_BB (bb) == first_bb); > sched_rgn_n_insns += sched_n_insns; > realloc_bb_state_array (saved_last_basic_block); > - f = find_fallthru_edge (last_bb->succs); > - if (f > - && (!f->probability.initialized_p () > - || (f->probability.to_reg_br_prob_base () * 100 > - / REG_BR_PROB_BASE > - >= param_sched_state_edge_prob_cutoff))) > - { > - memcpy (bb_state[f->dest->index], curr_state, > - dfa_state_size); > - if (sched_verbose >= 5) > - fprintf (sched_dump, "saving state for edge %d->%d\n", > - f->src->index, f->dest->index); > - } > + save_state_for_fallthru_edge (last_bb, curr_state); > } > else > { > diff --git a/gcc/testsuite/gcc.target/powerpc/pr105586.c b/gcc/testsuite/gcc.target/powerpc/pr105586.c > new file mode 100644 > index 00000000000..bd397f58bc0 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/powerpc/pr105586.c > @@ -0,0 +1,19 @@ > +/* { dg-options "-mdejagnu-tune=power4 -O2 -fcompare-debug -fno-if-conversion -fno-guess-branch-probability" } */ > + > +extern int bar(int i); > + > +typedef unsigned long u64; > +int g; > + > +__int128 h; > + > +void > +foo(int a, int b) { > + int i; > + char u8_1 = g, u8_3 = a; > + u64 u64_1 = bar(0), u64_3 = u8_3 * u64_1; > + __int128 u128_1 = h ^ __builtin_expect(i, 0); > + u64 u64_4 = u64_1 << ((__builtin_sub_overflow_p(0, u8_1, (u64)0) ^ u128_1) & 8); > + u64 u64_r = b + u64_3 + u64_4; > + bar((short)u64_r + u8_3); > +}
On 8/23/2022 5:49 AM, Surya Kumari Jangala via Gcc-patches wrote: > sched1: Fix -fcompare-debug issue in schedule_region [PR105586] > > In schedule_region(), a basic block that does not contain any real insns > is not scheduled and the dfa state at the entry of the bb is not copied > to the fallthru basic block. However a DEBUG insn is treated as a real > insn, and if a bb contains non-real insns and a DEBUG insn, it's dfa > state is copied to the fallthru bb. This was resulting in > -fcompare-debug failure as the incoming dfa state of the fallthru block > is different with -g. We should always copy the dfa state of a bb to > it's fallthru bb even if the bb does not contain real insns. > > 2022-08-22 Surya Kumari Jangala <jskumari@linux.ibm.com> > > gcc/ > PR rtl-optimization/105586 > * sched-rgn.cc (schedule_region): Always copy dfa state to > fallthru block. > > gcc/testsuite/ > PR rtl-optimization/105586 > * gcc.target/powerpc/pr105586.c: New test. Interesting. We may have stumbled over this bug internally a little while ago -- not from a compare-debug standpoint, but from a "why isn't the processor state copied to the fallthru block" point of view. I had it on my to investigate list, but hadn't gotten around to it yet. I think there were requests for ChangeLog updates and a function comment for save_state_for_fallthru_edge. OK with those updates. jeff
Hi Jeff, Richard, Thank you for reviewing the patch! I have committed the patch to the gcc repo. Can I backport this patch to prior versions of gcc, as this is an easy patch to backport and the issue exists in prior versions too? Regards, Surya On 31/08/22 9:09 pm, Jeff Law via Gcc-patches wrote: > > > On 8/23/2022 5:49 AM, Surya Kumari Jangala via Gcc-patches wrote: >> sched1: Fix -fcompare-debug issue in schedule_region [PR105586] >> >> In schedule_region(), a basic block that does not contain any real insns >> is not scheduled and the dfa state at the entry of the bb is not copied >> to the fallthru basic block. However a DEBUG insn is treated as a real >> insn, and if a bb contains non-real insns and a DEBUG insn, it's dfa >> state is copied to the fallthru bb. This was resulting in >> -fcompare-debug failure as the incoming dfa state of the fallthru block >> is different with -g. We should always copy the dfa state of a bb to >> it's fallthru bb even if the bb does not contain real insns. >> >> 2022-08-22 Surya Kumari Jangala <jskumari@linux.ibm.com> >> >> gcc/ >> PR rtl-optimization/105586 >> * sched-rgn.cc (schedule_region): Always copy dfa state to >> fallthru block. >> >> gcc/testsuite/ >> PR rtl-optimization/105586 >> * gcc.target/powerpc/pr105586.c: New test. > Interesting. We may have stumbled over this bug internally a little while ago -- not from a compare-debug standpoint, but from a "why isn't the processor state copied to the fallthru block" point of view. I had it on my to investigate list, but hadn't gotten around to it yet. > > I think there were requests for ChangeLog updates and a function comment for save_state_for_fallthru_edge. OK with those updates. > > jeff >
On Tue, Sep 20, 2022 at 9:18 AM Surya Kumari Jangala via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > Hi Jeff, Richard, > Thank you for reviewing the patch! > I have committed the patch to the gcc repo. > Can I backport this patch to prior versions of gcc, as this is an easy patch to backport and the issue exists in prior versions too? It doesn't seem to be a regression so I'd error on the safe side here. Richard. > Regards, > Surya > > > On 31/08/22 9:09 pm, Jeff Law via Gcc-patches wrote: > > > > > > On 8/23/2022 5:49 AM, Surya Kumari Jangala via Gcc-patches wrote: > >> sched1: Fix -fcompare-debug issue in schedule_region [PR105586] > >> > >> In schedule_region(), a basic block that does not contain any real insns > >> is not scheduled and the dfa state at the entry of the bb is not copied > >> to the fallthru basic block. However a DEBUG insn is treated as a real > >> insn, and if a bb contains non-real insns and a DEBUG insn, it's dfa > >> state is copied to the fallthru bb. This was resulting in > >> -fcompare-debug failure as the incoming dfa state of the fallthru block > >> is different with -g. We should always copy the dfa state of a bb to > >> it's fallthru bb even if the bb does not contain real insns. > >> > >> 2022-08-22 Surya Kumari Jangala <jskumari@linux.ibm.com> > >> > >> gcc/ > >> PR rtl-optimization/105586 > >> * sched-rgn.cc (schedule_region): Always copy dfa state to > >> fallthru block. > >> > >> gcc/testsuite/ > >> PR rtl-optimization/105586 > >> * gcc.target/powerpc/pr105586.c: New test. > > Interesting. We may have stumbled over this bug internally a little while ago -- not from a compare-debug standpoint, but from a "why isn't the processor state copied to the fallthru block" point of view. I had it on my to investigate list, but hadn't gotten around to it yet. > > > > I think there were requests for ChangeLog updates and a function comment for save_state_for_fallthru_edge. OK with those updates. > > > > jeff > >
Hi Richard, On 21/09/22 1:03 pm, Richard Biener wrote: > On Tue, Sep 20, 2022 at 9:18 AM Surya Kumari Jangala via Gcc-patches > <gcc-patches@gcc.gnu.org> wrote: >> >> Hi Jeff, Richard, >> Thank you for reviewing the patch! >> I have committed the patch to the gcc repo. >> Can I backport this patch to prior versions of gcc, as this is an easy patch to backport and the issue exists in prior versions too? > > It doesn't seem to be a regression so I'd error on the safe side here. Can you please clarify, should this patch be backported? It is not very clear what "safe side" means here. Thanks! Surya > > Richard. > >> Regards, >> Surya >> >> >> On 31/08/22 9:09 pm, Jeff Law via Gcc-patches wrote: >>> >>> >>> On 8/23/2022 5:49 AM, Surya Kumari Jangala via Gcc-patches wrote: >>>> sched1: Fix -fcompare-debug issue in schedule_region [PR105586] >>>> >>>> In schedule_region(), a basic block that does not contain any real insns >>>> is not scheduled and the dfa state at the entry of the bb is not copied >>>> to the fallthru basic block. However a DEBUG insn is treated as a real >>>> insn, and if a bb contains non-real insns and a DEBUG insn, it's dfa >>>> state is copied to the fallthru bb. This was resulting in >>>> -fcompare-debug failure as the incoming dfa state of the fallthru block >>>> is different with -g. We should always copy the dfa state of a bb to >>>> it's fallthru bb even if the bb does not contain real insns. >>>> >>>> 2022-08-22 Surya Kumari Jangala <jskumari@linux.ibm.com> >>>> >>>> gcc/ >>>> PR rtl-optimization/105586 >>>> * sched-rgn.cc (schedule_region): Always copy dfa state to >>>> fallthru block. >>>> >>>> gcc/testsuite/ >>>> PR rtl-optimization/105586 >>>> * gcc.target/powerpc/pr105586.c: New test. >>> Interesting. We may have stumbled over this bug internally a little while ago -- not from a compare-debug standpoint, but from a "why isn't the processor state copied to the fallthru block" point of view. I had it on my to investigate list, but hadn't gotten around to it yet. >>> >>> I think there were requests for ChangeLog updates and a function comment for save_state_for_fallthru_edge. OK with those updates. >>> >>> jeff >>>
On Tue, Nov 8, 2022 at 5:37 PM Surya Kumari Jangala <jskumari@linux.vnet.ibm.com> wrote: > > Hi Richard, > > On 21/09/22 1:03 pm, Richard Biener wrote: > > On Tue, Sep 20, 2022 at 9:18 AM Surya Kumari Jangala via Gcc-patches > > <gcc-patches@gcc.gnu.org> wrote: > >> > >> Hi Jeff, Richard, > >> Thank you for reviewing the patch! > >> I have committed the patch to the gcc repo. > >> Can I backport this patch to prior versions of gcc, as this is an easy patch to backport and the issue exists in prior versions too? > > > > It doesn't seem to be a regression so I'd error on the safe side here. > > Can you please clarify, should this patch be backported? It is not very clear what "safe side" means here. Not backporting is the safe side. Richard. > Thanks! > Surya > > > > > Richard. > > > >> Regards, > >> Surya > >> > >> > >> On 31/08/22 9:09 pm, Jeff Law via Gcc-patches wrote: > >>> > >>> > >>> On 8/23/2022 5:49 AM, Surya Kumari Jangala via Gcc-patches wrote: > >>>> sched1: Fix -fcompare-debug issue in schedule_region [PR105586] > >>>> > >>>> In schedule_region(), a basic block that does not contain any real insns > >>>> is not scheduled and the dfa state at the entry of the bb is not copied > >>>> to the fallthru basic block. However a DEBUG insn is treated as a real > >>>> insn, and if a bb contains non-real insns and a DEBUG insn, it's dfa > >>>> state is copied to the fallthru bb. This was resulting in > >>>> -fcompare-debug failure as the incoming dfa state of the fallthru block > >>>> is different with -g. We should always copy the dfa state of a bb to > >>>> it's fallthru bb even if the bb does not contain real insns. > >>>> > >>>> 2022-08-22 Surya Kumari Jangala <jskumari@linux.ibm.com> > >>>> > >>>> gcc/ > >>>> PR rtl-optimization/105586 > >>>> * sched-rgn.cc (schedule_region): Always copy dfa state to > >>>> fallthru block. > >>>> > >>>> gcc/testsuite/ > >>>> PR rtl-optimization/105586 > >>>> * gcc.target/powerpc/pr105586.c: New test. > >>> Interesting. We may have stumbled over this bug internally a little while ago -- not from a compare-debug standpoint, but from a "why isn't the processor state copied to the fallthru block" point of view. I had it on my to investigate list, but hadn't gotten around to it yet. > >>> > >>> I think there were requests for ChangeLog updates and a function comment for save_state_for_fallthru_edge. OK with those updates. > >>> > >>> jeff > >>>
diff --git a/gcc/sched-rgn.cc b/gcc/sched-rgn.cc index 0dc2a8f2851..6c9202c0b2b 100644 --- a/gcc/sched-rgn.cc +++ b/gcc/sched-rgn.cc @@ -3082,6 +3082,24 @@ free_bb_state_array (void) bb_state = NULL; } +static void +save_state_for_fallthru_edge (basic_block last_bb, state_t state) +{ + edge f = find_fallthru_edge (last_bb->succs); + if (f + && (!f->probability.initialized_p () + || (f->probability.to_reg_br_prob_base () * 100 + / REG_BR_PROB_BASE + >= param_sched_state_edge_prob_cutoff))) + { + memcpy (bb_state[f->dest->index], state, + dfa_state_size); + if (sched_verbose >= 5) + fprintf (sched_dump, "saving state for edge %d->%d\n", + f->src->index, f->dest->index); + } +} + /* Schedule a region. A region is either an inner loop, a loop-free subroutine, or a single basic block. Each bb in the region is scheduled after its flow predecessors. */ @@ -3155,6 +3173,7 @@ schedule_region (int rgn) if (no_real_insns_p (head, tail)) { gcc_assert (first_bb == last_bb); + save_state_for_fallthru_edge (last_bb, bb_state[first_bb->index]); continue; } @@ -3173,26 +3192,13 @@ schedule_region (int rgn) curr_bb = first_bb; if (dbg_cnt (sched_block)) { - edge f; int saved_last_basic_block = last_basic_block_for_fn (cfun); schedule_block (&curr_bb, bb_state[first_bb->index]); gcc_assert (EBB_FIRST_BB (bb) == first_bb); sched_rgn_n_insns += sched_n_insns; realloc_bb_state_array (saved_last_basic_block); - f = find_fallthru_edge (last_bb->succs); - if (f - && (!f->probability.initialized_p () - || (f->probability.to_reg_br_prob_base () * 100 - / REG_BR_PROB_BASE - >= param_sched_state_edge_prob_cutoff))) - { - memcpy (bb_state[f->dest->index], curr_state, - dfa_state_size); - if (sched_verbose >= 5) - fprintf (sched_dump, "saving state for edge %d->%d\n", - f->src->index, f->dest->index); - } + save_state_for_fallthru_edge (last_bb, curr_state); } else { diff --git a/gcc/testsuite/gcc.target/powerpc/pr105586.c b/gcc/testsuite/gcc.target/powerpc/pr105586.c new file mode 100644 index 00000000000..bd397f58bc0 --- /dev/null +++ b/gcc/testsuite/gcc.target/powerpc/pr105586.c @@ -0,0 +1,19 @@ +/* { dg-options "-mdejagnu-tune=power4 -O2 -fcompare-debug -fno-if-conversion -fno-guess-branch-probability" } */ + +extern int bar(int i); + +typedef unsigned long u64; +int g; + +__int128 h; + +void +foo(int a, int b) { + int i; + char u8_1 = g, u8_3 = a; + u64 u64_1 = bar(0), u64_3 = u8_3 * u64_1; + __int128 u128_1 = h ^ __builtin_expect(i, 0); + u64 u64_4 = u64_1 << ((__builtin_sub_overflow_p(0, u8_1, (u64)0) ^ u128_1) & 8); + u64 u64_r = b + u64_3 + u64_4; + bar((short)u64_r + u8_3); +}