From patchwork Mon Dec 4 17:30:45 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Richard Sandiford X-Patchwork-Id: 81295 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 52F9D384DEE7 for ; Mon, 4 Dec 2023 17:31:05 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id 1F19F3858C41 for ; Mon, 4 Dec 2023 17:30:48 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 1F19F3858C41 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=arm.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 1F19F3858C41 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=217.140.110.172 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1701711049; cv=none; b=sOiW/QT21T6cLE5owL9jxLPHwlmjUWCqy99WdpcrXc6Hxg9JpeUwokvp0MaYNOVDCenPpLMU5Sob0hkhJI/aUn6t88mRmptz9v6osOjujNDdD0qWcvLc7phcb1aoYlQqyq26jmGxsGsn/2wdKq9MBWVsxuxuwjit+xetCh3wGfY= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1701711049; c=relaxed/simple; bh=0PwPu0bPy7vE/vMnL3DEIEuwtfmLpA3U9WwXidZw8RM=; h=From:To:Subject:Date:Message-ID:MIME-Version; b=VwIR2H1HvmjbjfTTfwg1b/0ibuyTXCyIh5GJeP697wI4dvovtdmZJls11J48C1VJIcsW0t/AEO48zAXy94Du65wPznG2pZ4WENcTeCrHoF4YevfQHpz81ccg5xl03+W+kJAQmfeICoFe2qRSTMZLMx5GQa7MUlt6SSfScAJSNgw= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id E85BB13D5; Mon, 4 Dec 2023 09:31:34 -0800 (PST) Received: from localhost (e121540-lin.manchester.arm.com [10.32.110.72]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id F09533F6C4; Mon, 4 Dec 2023 09:30:46 -0800 (PST) From: Richard Sandiford To: Jakub Jelinek Mail-Followup-To: Jakub Jelinek , Eric Botcazou , Segher Boessenkool , Jeff Law , gcc-patches@gcc.gnu.org, richard.sandiford@arm.com Cc: Eric Botcazou , Segher Boessenkool , Jeff Law , gcc-patches@gcc.gnu.org Subject: [PATCH] Maintain a validity flag for REG_UNUSED notes [PR112760] (was Re: [PATCH] pro_and_epilogue: Call df_note_add_problem () if SHRINK_WRAPPING_ENABLED [PR112760]) References: Date: Mon, 04 Dec 2023 17:30:45 +0000 In-Reply-To: (Richard Sandiford's message of "Mon, 04 Dec 2023 10:59:50 +0000") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 X-Spam-Status: No, score=-22.2 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_NONE, KAM_DMARC_STATUS, KAM_LAZY_DOMAIN_SECURITY, 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.30 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Richard Sandiford writes: > Jakub Jelinek writes: >> On Sat, Dec 02, 2023 at 11:04:04AM +0000, Richard Sandiford wrote: >>> I still maintain that so much stuff relies on the lack of false-positive >>> REG_UNUSED notes that (whatever the intention might have been) we need >>> to prevent the false positive. Like Andrew says, any use of single_set >>> is suspect if there's a REG_UNUSED note for something that is in fact used. >> >> The false positive REG_UNUSED in that case comes from >> (insn 15 14 35 2 (set (reg:CCZ 17 flags) >> (compare:CCZ (reg:DI 0 ax [111]) >> (reg:DI 1 dx [112]))) "pr112760.c":11:22 12 {*cmpdi_1} >> (expr_list:REG_UNUSED (reg:CCZ 17 flags) >> (nil))) >> (insn 35 15 36 2 (set (reg:CCZ 17 flags) >> (compare:CCZ (reg:DI 0 ax [111]) >> (reg:DI 1 dx [112]))) "pr112760.c":11:22 12 {*cmpdi_1} >> (expr_list:REG_DEAD (reg:DI 1 dx [112]) >> (expr_list:REG_DEAD (reg:DI 0 ax [111]) >> (nil)))) >> ... >> use of flags >> Haven't verified what causes the redundant comparison, but postreload cse >> then does: >> 110 if (!count && cselib_redundant_set_p (body)) >> 111 { >> 112 if (check_for_inc_dec (insn)) >> 113 delete_insn_and_edges (insn); >> 114 /* We're done with this insn. */ >> 115 goto done; >> 116 } >> So, we'd in such cases need to look up what instruction was the earlier >> setter and if it has REG_UNUSED note, drop it. > > Hmm, OK. I guess it's not as simple as I'd imagined. cselib does have > some code to track which instruction established which equivalence, > but it doesn't currently record what we want, and it would be difficult > to reuse that information here anyway. Something "simple" like a map of > register numbers to instructions, populated only for REG_UNUSED sets, > would be enough, and low overhead. But it's not very natural. > > Perhaps DF should maintain a flag to say "the current pass keeps > notes up-to-date", with the assumption being that any pass that > uses the notes problem does that. Then single_set and the > regcprop.cc uses can check that flag. > > I don't think it's worth adding the note problem to shrink-wrapping > just for the regcprop code. If we're prepared to take that compile-time > hit, we might as well run a proper (fast) DCE. Here's a patch that tries to do that. Boostrapped & regression tested on aarch64-linux-gnu. Also tested on x86_64-linux-gnu for the testcase. (I'll run full x86_64-linux-gnu testing overnight.) OK to install if that passes? Not an elegant fix, but it's probably too much to hope for one of those. Richard -------- PR112760 is a miscompilation caused by a stale, false-positive REG_UNUSED note. There were originally two consecutive, identical instructions that set the CC flags. The first originally had a REG_UNUSED note, but postreload later deleted the second in favour of the first, based on cselib_redundant_set_p. Although in principle it would be possible to remove the note when making the optimisation, the required bookkeeping wouldn't fit naturally into what cselib already does. Doing that would also arguably be a change of policy. This patch instead adds a global flag that says whether REG_UNUSED notes are trustworthy. The assumption is that any pass that calls df_note_add_problem cares about REG_UNUSED notes and will keep them sufficiently up-to-date to support the pass's use of things like single_set. gcc/ PR rtl-optimization/112760 * df.h (df_d::can_trust_reg_unused_notes): New member variable. * df-problems.cc (df_note_add_problem): Set can_trust_reg_unused_notes to true. * passes.cc (execute_one_pass): Clear can_trust_reg_unused_notes after each pass. * rtlanal.cc (single_set_2): Check can_trust_reg_unused_notes. * regcprop.cc (copyprop_hardreg_forward_1): Likewise. gcc/testsuite/ * gcc.dg/pr112760.c: New test. --- gcc/df-problems.cc | 1 + gcc/df.h | 4 ++++ gcc/passes.cc | 3 +++ gcc/regcprop.cc | 4 +++- gcc/rtlanal.cc | 8 ++++++-- gcc/testsuite/gcc.dg/pr112760.c | 22 ++++++++++++++++++++++ 6 files changed, 39 insertions(+), 3 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/pr112760.c diff --git a/gcc/df-problems.cc b/gcc/df-problems.cc index d2cfaf7f50f..d2eb95d35ad 100644 --- a/gcc/df-problems.cc +++ b/gcc/df-problems.cc @@ -3782,6 +3782,7 @@ void df_note_add_problem (void) { df_add_problem (&problem_NOTE); + df->can_trust_reg_unused_notes = true; } diff --git a/gcc/df.h b/gcc/df.h index 402657a7076..a405c000235 100644 --- a/gcc/df.h +++ b/gcc/df.h @@ -614,6 +614,10 @@ public: /* True if someone added or deleted something from regs_ever_live so that the entry and exit blocks need be reprocessed. */ bool redo_entry_and_exit; + + /* True if REG_UNUSED notes are up-to-date enough to be free of false + positives. */ + bool can_trust_reg_unused_notes; }; #define DF_SCAN_BB_INFO(BB) (df_scan_get_bb_info ((BB)->index)) diff --git a/gcc/passes.cc b/gcc/passes.cc index 6f894a41d22..0313d0e3689 100644 --- a/gcc/passes.cc +++ b/gcc/passes.cc @@ -2640,6 +2640,9 @@ execute_one_pass (opt_pass *pass) /* Do it! */ todo_after = pass->execute (cfun); + if (df) + df->can_trust_reg_unused_notes = false; + if (todo_after & TODO_discard_function) { /* Stop timevar. */ diff --git a/gcc/regcprop.cc b/gcc/regcprop.cc index a75af2ba7e3..03df9a7cf4f 100644 --- a/gcc/regcprop.cc +++ b/gcc/regcprop.cc @@ -820,6 +820,7 @@ copyprop_hardreg_forward_1 (basic_block bb, struct value_data *vd) if (set && !RTX_FRAME_RELATED_P (insn) && NONJUMP_INSN_P (insn) + && df->can_trust_reg_unused_notes && !may_trap_p (set) && find_reg_note (insn, REG_UNUSED, SET_DEST (set)) && !side_effects_p (SET_SRC (set)) @@ -878,7 +879,8 @@ copyprop_hardreg_forward_1 (basic_block bb, struct value_data *vd) would clobbers. */ for (link = REG_NOTES (insn); link; link = XEXP (link, 1)) { - if (REG_NOTE_KIND (link) == REG_UNUSED) + if (df->can_trust_reg_unused_notes + && REG_NOTE_KIND (link) == REG_UNUSED) { kill_value (XEXP (link, 0), vd); /* Furthermore, if the insn looked like a single-set, diff --git a/gcc/rtlanal.cc b/gcc/rtlanal.cc index 87267ee3b88..42b07cd9634 100644 --- a/gcc/rtlanal.cc +++ b/gcc/rtlanal.cc @@ -1572,7 +1572,9 @@ single_set_2 (const rtx_insn *insn, const_rtx pat) sets are found in the insn, we check them. */ if (!set_verified) { - if (find_reg_note (insn, REG_UNUSED, SET_DEST (set)) + if (df + && df->can_trust_reg_unused_notes + && find_reg_note (insn, REG_UNUSED, SET_DEST (set)) && !side_effects_p (set)) set = NULL; else @@ -1580,7 +1582,9 @@ single_set_2 (const rtx_insn *insn, const_rtx pat) } if (!set) set = sub, set_verified = 0; - else if (!find_reg_note (insn, REG_UNUSED, SET_DEST (sub)) + else if (!df + || !df->can_trust_reg_unused_notes + || !find_reg_note (insn, REG_UNUSED, SET_DEST (sub)) || side_effects_p (sub)) return NULL_RTX; break; diff --git a/gcc/testsuite/gcc.dg/pr112760.c b/gcc/testsuite/gcc.dg/pr112760.c new file mode 100644 index 00000000000..b4ec70e4701 --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr112760.c @@ -0,0 +1,22 @@ +/* PR rtl-optimization/112760 */ +/* { dg-do run } */ +/* { dg-options "-O2 -fno-dce -fno-guess-branch-probability --param=max-cse-insns=0" } */ +/* { dg-additional-options "-m8bit-idiv -mavx" { target i?86-*-* x86_64-*-* } } */ + +unsigned g; + +__attribute__((__noipa__)) unsigned short +foo (unsigned short a, unsigned short b) +{ + unsigned short x = __builtin_add_overflow_p (a, g, (unsigned short) 0); + g -= g / b; + return x; +} + +int +main () +{ + unsigned short x = foo (40, 6); + if (x != 0) + __builtin_abort (); +}