From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lj1-x22d.google.com (mail-lj1-x22d.google.com [IPv6:2a00:1450:4864:20::22d]) by sourceware.org (Postfix) with ESMTPS id E88873858C60 for ; Thu, 2 Feb 2023 12:30:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org E88873858C60 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-lj1-x22d.google.com with SMTP id h17so1711543ljq.4 for ; Thu, 02 Feb 2023 04:30:15 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=to:subject:message-id:date:from:in-reply-to:references:mime-version :from:to:cc:subject:date:message-id:reply-to; bh=ZpOcUVndrUCEbbZUe9qilh6BaejqRwCoat39WPgsE0A=; b=czL8KKtMtd/RxQIrVzHRjTrnzP4IFtU6BN8QrxNmY0fjkQ90OMF01YAL2pp7+FJCLX QbSOgAZc+IL7h/EAFiiIL07GVnSQWJWMhIdizGnqDeZiGGSH0JzOz+s/Re7wFTRlvHrJ OJiBxR/FpnoSWLSsE9ISnP4oforN+tv8nD8wgvRw2hz5g24VfKAtypugwPL0391+j/BX 7z+f9CvzgjgCNfBxSWUJMAEY8YgIbMXgk99iUiVtlcNMigJ8wuYF3rgX3BFrSM0Y8rby rdirP9xpJz8qXrCfRZ1lDUeHTB5BpuCwV+OVQErohZCHu3J8DCArAd9yDrZ/0mFCl3gK 3Z8w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=to:subject:message-id:date:from:in-reply-to:references:mime-version :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=ZpOcUVndrUCEbbZUe9qilh6BaejqRwCoat39WPgsE0A=; b=5bpLS6234QPavb8mGYSuczeNcIUXFLUcPcBlM1cCxGAEuqQWlSDuAFInf1DSMi3Y3+ MQxDcgTTBnhMXP/D1G0KZBONYrRVGlzcCX1DdOQpyQJh4hfMxkBIXGwO4TGeAGo1Bwql fiR8OoM/oFp1Re0UHssIQH8MP/Myyx1I3tcTYrCj0BLhQnDYeQJm4o95XfUGdldvpbse k5qwXZ0ZwVO0W+j1uvHQdof54Ki3USkkZIT2kM/T0DhuwKqphMMoDiKxCsIxzPHp1FWp MM8nadOxoH0SgqOp3vkZ4ouJdy5LdbjlZYA91E3a4466YxsugNkm2sW6WlGkbvx8pBGy 2oyA== X-Gm-Message-State: AO0yUKXk9T6tSrljZ6V4NA69Zz5+zbBAiVGqIfnN2u93iicwtlykccry y4+Q5/4SYubCoVflTDPKtAxNZdbeJQbJtfsMr8Q= X-Google-Smtp-Source: AK7set9nU5yO1ETIYyWHPsK4mbhLxlHdFjZcp+Qt91SAc+epAd7xC8A1NbBFtj8uVWXMk2uhyOf816Cvgu323JrzE/4= X-Received: by 2002:a2e:2a82:0:b0:28e:d61:5a46 with SMTP id q124-20020a2e2a82000000b0028e0d615a46mr1070903ljq.32.1675341014234; Thu, 02 Feb 2023 04:30:14 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: Richard Biener Date: Thu, 2 Feb 2023 13:30:01 +0100 Message-ID: Subject: Re: [PATCH] rtl-ssa: Fix splitting of clobber groups [PR108508] To: Richard Sandiford , gcc-patches@gcc.gnu.org Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-7.3 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Thu, Feb 2, 2023 at 10:49 AM Richard Sandiford via Gcc-patches wrote: > > Since rtl-ssa isn't a real/native SSA representation, it has > to honour the constraints of the underlying rtl representation. > Part of this involves maintaining an rpo list of definitions > for each rtl register, backed by a splay tree where necessary > for quick lookup/insertion. > > However, clobbers of a register don't act as barriers to > other clobbers of a register. E.g. it's possible to move one > flag-clobbering instruction across an arbitrary number of other > flag-clobbering instructions. In order to allow passes to do > that without quadratic complexity, the splay tree groups all > consecutive clobbers into groups, with only the group being > entered into the splay tree. These groups in turn have an > internal splay tree of clobbers where necessary. > > This means that, if we insert a new definition and use into > the middle of a sea of clobbers, we need to split the clobber > group into two groups. This was quite a difficult condition > to trigger during development, and the PR shows that the code > to handle it had (at least) two bugs. > > First, the process involves searching the clobber tree for > the split point. This search can give either the previous > clobber (which will belong to the first of the split groups) > or the next clobber (which will belong to the second of the > split groups). The code for the former case handled the > split correctly but the code for the latter case didn't. > > Second, I'd forgotten to add the second clobber group to the > main splay tree. :-( > > Tested on aarch64-linux-gnu & x86_64-linux-gnu. OK for trunk > & GCC 12? Although the testcase is "only" a regression from GCC 12, > I think the rtl-ssa patch should be backported to GCC 11 too. OK. Thanks, Richard. > Richard > > > gcc/ > PR rtl-optimization/108508 > * rtl-ssa/accesses.cc (function_info::split_clobber_group): When > the splay tree search gives the first clobber in the second group, > make sure that the root of the first clobber group is updated > correctly. Enter the new clobber group into the definition splay > tree. > > gcc/testsuite/ > PR rtl-optimization/108508 > * gcc.target/aarch64/pr108508.c: New test. > --- > gcc/rtl-ssa/accesses.cc | 14 ++++++++--- > gcc/testsuite/gcc.target/aarch64/pr108508.c | 28 +++++++++++++++++++++ > 2 files changed, 38 insertions(+), 4 deletions(-) > create mode 100644 gcc/testsuite/gcc.target/aarch64/pr108508.c > > diff --git a/gcc/rtl-ssa/accesses.cc b/gcc/rtl-ssa/accesses.cc > index 03b9a475d3b..f12b5f4dd77 100644 > --- a/gcc/rtl-ssa/accesses.cc > +++ b/gcc/rtl-ssa/accesses.cc > @@ -794,23 +794,26 @@ function_info::merge_clobber_groups (clobber_info *clobber1, > // GROUP spans INSN, and INSN now sets the resource that GROUP clobbers. > // Split GROUP around INSN and return the clobber that comes immediately > // before INSN. > +// > +// The resource that GROUP clobbers is known to have an associated > +// splay tree. > clobber_info * > function_info::split_clobber_group (clobber_group *group, insn_info *insn) > { > // Search for either the previous or next clobber in the group. > // The result is less than zero if CLOBBER should come before NEIGHBOR > // or greater than zero if CLOBBER should come after NEIGHBOR. > - int comparison = lookup_clobber (group->m_clobber_tree, insn); > + clobber_tree &tree1 = group->m_clobber_tree; > + int comparison = lookup_clobber (tree1, insn); > gcc_checking_assert (comparison != 0); > - clobber_info *neighbor = group->m_clobber_tree.root (); > + clobber_info *neighbor = tree1.root (); > > - clobber_tree tree1, tree2; > + clobber_tree tree2; > clobber_info *prev; > clobber_info *next; > if (comparison > 0) > { > // NEIGHBOR is the last clobber in what will become the first group. > - tree1 = neighbor; > tree2 = tree1.split_after_root (); > prev = neighbor; > next = as_a (prev->next_def ()); > @@ -843,6 +846,9 @@ function_info::split_clobber_group (clobber_group *group, insn_info *insn) > tree2->set_group (group2); > last_clobber->set_group (group2); > > + // Insert GROUP2 into the splay tree as an immediate successor of GROUP1. > + def_splay_tree::insert_child (group1, 1, group2); > + > return prev; > } > > diff --git a/gcc/testsuite/gcc.target/aarch64/pr108508.c b/gcc/testsuite/gcc.target/aarch64/pr108508.c > new file mode 100644 > index 00000000000..e97896b6a1b > --- /dev/null > +++ b/gcc/testsuite/gcc.target/aarch64/pr108508.c > @@ -0,0 +1,28 @@ > +/* { dg-options "-O3 -fharden-conditional-branches -fno-dce -fno-guess-branch-probability" } */ > + > +#include > + > +int > +test_vld3q_lane_f64 (void) > +{ > + float64x2x3_t vectors; > + float64_t temp[2]; > + int i, j; > + > + for (i = 0; i < 3; i++) > + { > + vst1q_f64 (temp, vectors.val[i]); > + for (j = 0; j < 2; j++) > + if (temp[j]) > + return 1; > + } > + > + return 0; > +} > + > +void > +foo (void) > +{ > + if (test_vld3q_lane_f64 () || test_vld3q_lane_f64 ()) > + __builtin_abort (); > +} > -- > 2.25.1 >