Message ID | 20220428191704.89750-1-iain@sandoe.co.uk |
---|---|
State | New |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> 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 625763857340 for <patchwork@sourceware.org>; Thu, 28 Apr 2022 19:17:48 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 625763857340 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1651173468; bh=DtN/9wF1B3gbYB1+ABGtTIiIwWHdvsZY6eKIarQ56Bc=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:Cc:From; b=KJ0uFcZDioCX1kk2bUkZSzwK9EebkmoJurkZJCCqHCfAUagcMS1lbzisWTtmtKewL oH5NvaeL/KUIXBWpsiPrrcwoqvpSVo9g5kWf4IldaGXcDDLUfmd/InAMurQpywoZMk Rb2O4i99QsMZ5dZ53DNhmkrySVfJq0fCi8+Gw2Ps= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mail-wr1-x433.google.com (mail-wr1-x433.google.com [IPv6:2a00:1450:4864:20::433]) by sourceware.org (Postfix) with ESMTPS id A9B133858D28 for <gcc-patches@gcc.gnu.org>; Thu, 28 Apr 2022 19:17:12 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A9B133858D28 Received: by mail-wr1-x433.google.com with SMTP id x18so8074452wrc.0 for <gcc-patches@gcc.gnu.org>; Thu, 28 Apr 2022 12:17:12 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:to:cc:subject:date:message-id:reply-to :mime-version:content-transfer-encoding; bh=DtN/9wF1B3gbYB1+ABGtTIiIwWHdvsZY6eKIarQ56Bc=; b=XrwPyAXIVa538ckc/3vxbJCQUtt1gNgpPfI28cvSldyCcsPGz63N1nYAuNAn3qyMeg YBASo4TtdqmIbyQyO1+2czvJemoBFzQYSj5QBGJmBXORPxaNVlWeJdtQ2iZDlRLgLPAz ChMb2WfM0hskpvkUPRvvCAUDBSOG0TzaWvM9HLcHrtWpkUUH8ptlHdQCfTBx9840VDL+ Lt7crKJPW6v9MKfHGU8FD4XBr57vFyQL2c9C5kfaW9R0k+2Z9AHfQ0HJdEjm28R+f5Vz bU48qWnVjylj+/unFZHCiaVIUUBL915adaFOgqRsZsKNW++zS31Twjc6eXKqe4FmXJAq mI1g== X-Gm-Message-State: AOAM532ajxhn8eZRsXS+uqNHlWToWBiibV/aTVJ22RVLMIonl47w/d7c n/hatIJukpGpjm5MCx089Xy5zD0SPMg= X-Google-Smtp-Source: ABdhPJxcUwbpnaVo9hSbqQc951L256RAEMpEj1DHYorNz2hA8mDNO9qkO7uIhIbfX1+TujpGi4uhmg== X-Received: by 2002:a5d:6f0c:0:b0:20a:7db1:3408 with SMTP id ay12-20020a5d6f0c000000b0020a7db13408mr27749098wrb.267.1651173431448; Thu, 28 Apr 2022 12:17:11 -0700 (PDT) Received: from localhost.localdomain (host81-138-1-83.in-addr.btopenworld.com. [81.138.1.83]) by smtp.gmail.com with ESMTPSA id n5-20020a05600c3b8500b00393fb9923basm666046wms.39.2022.04.28.12.17.10 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Thu, 28 Apr 2022 12:17:10 -0700 (PDT) X-Google-Original-From: Iain Sandoe <iain@sandoe.co.uk> To: gcc-patches@gcc.gnu.org Subject: [PATCH] c++, coroutines: Partial reversion of r12-8308-g15a176a833f23e [PR105426]. Date: Thu, 28 Apr 2022 20:17:04 +0100 Message-Id: <20220428191704.89750-1-iain@sandoe.co.uk> X-Mailer: git-send-email 2.24.3 (Apple Git-128) MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-8.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, 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 <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: Iain Sandoe via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: iain@sandoe.co.uk Cc: Iain Sandoe <iains.gcc@gmail.com>, jakub@redhat.com Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
c++, coroutines: Partial reversion of r12-8308-g15a176a833f23e [PR105426].
|
|
Commit Message
Iain Sandoe
April 28, 2022, 7:17 p.m. UTC
The changes to fix PR 105287 included a tightening of the constraints on which
variables are promoted to frame copies. This has exposed that we are failing
to name some variables that should be promoted.
The long-term fix is to address the cases where the naming has been missed,
but for the short-term (and for the GCC-12 branch) backing out the additional
constraint is proposed.
This makes the fix for PR 105287 more conservative.
tested on x86_64-darwin, with the reproducer from the PR which now produces
the correct output for both optimised and unoptimised code.
OK for master?
OK for GCC-12?
thanks,
Iain
Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>
PR c++/105426
gcc/cp/ChangeLog:
* coroutines.cc (register_local_var_uses): Allow promotion of unnamed
temporaries to coroutine frame copies.
---
gcc/cp/coroutines.cc | 3 +++
1 file changed, 3 insertions(+)
Comments
On Thu, Apr 28, 2022 at 08:17:04PM +0100, Iain Sandoe wrote: > Signed-off-by: Iain Sandoe <iain@sandoe.co.uk> > > PR c++/105426 > > gcc/cp/ChangeLog: > > * coroutines.cc (register_local_var_uses): Allow promotion of unnamed > temporaries to coroutine frame copies. > --- > gcc/cp/coroutines.cc | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc > index 551ddc9cc41..2e393b2cddc 100644 > --- a/gcc/cp/coroutines.cc > +++ b/gcc/cp/coroutines.cc > @@ -3973,6 +3973,9 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d) > else if (lvname != NULL_TREE) > buf = xasprintf ("%s_%u_%u", IDENTIFIER_POINTER (lvname), > lvd->nest_depth, lvd->bind_indx); > + else > + buf = xasprintf ("_D%u_%u_%u", DECL_UID (lvar), lvd->nest_depth, > + lvd->bind_indx); > /* TODO: Figure out if we should build a local type that has any > excess alignment or size from the original decl. */ > if (buf) This isn't going to play well with -fcompare-debug. We don't guarantee same DECL_UID values between -g and -g0, just that when two decls are created with both -g and -g0 they compare the same, but with -g the gap in between them could be larger. So names that include DECL_UID will be often different between -g and -g0. Can't the FIELD_DECL be instead nameless? Say change coro_make_frame_entry to do tree id = name ? get_identifier (name) : NULL_TREE; instead of tree id = get_identifier (name); ? Jakub
> On 28 Apr 2022, at 20:26, Jakub Jelinek <jakub@redhat.com> wrote: > > On Thu, Apr 28, 2022 at 08:17:04PM +0100, Iain Sandoe wrote: >> Signed-off-by: Iain Sandoe <iain@sandoe.co.uk> >> >> PR c++/105426 >> >> gcc/cp/ChangeLog: >> >> * coroutines.cc (register_local_var_uses): Allow promotion of unnamed >> temporaries to coroutine frame copies. >> --- >> gcc/cp/coroutines.cc | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc >> index 551ddc9cc41..2e393b2cddc 100644 >> --- a/gcc/cp/coroutines.cc >> +++ b/gcc/cp/coroutines.cc >> @@ -3973,6 +3973,9 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d) >> else if (lvname != NULL_TREE) >> buf = xasprintf ("%s_%u_%u", IDENTIFIER_POINTER (lvname), >> lvd->nest_depth, lvd->bind_indx); >> + else >> + buf = xasprintf ("_D%u_%u_%u", DECL_UID (lvar), lvd->nest_depth, >> + lvd->bind_indx); >> /* TODO: Figure out if we should build a local type that has any >> excess alignment or size from the original decl. */ >> if (buf) > > This isn't going to play well with -fcompare-debug. > We don't guarantee same DECL_UID values between -g and -g0, just that > when two decls are created with both -g and -g0 they compare the same, > but with -g the gap in between them could be larger. > So names that include DECL_UID will be often different between -g and -g0. that’s somewhat unfortunate (having the UIDs in the frame and the gimple is quite helpful to debugging). I guess I was not expecting a difference this early in the FE (we are before initial folding / constexpr stuff). FWIW, I ran the coroutines and coroutines torture testsuites with -fcompare-debug. There are 5 fails, and those are unaffected by whether the field is made nameless as suggested below (so something else to investigate). > Can't the FIELD_DECL be instead nameless? Say change coro_make_frame_entry > to do > tree id = name ? get_identifier (name) : NULL_TREE; > instead of > tree id = get_identifier (name); Unfortunately, that does not work - although I cannot see anything in the coroutines code that would cause it to fail - perhaps something is not playing nicely with DECL_VALUE_EXPRs on unnamed temps? how about the following, which uniques the names by bind scope, scope nest and then sequence within that? Iain diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc index 2e393b2cddc..1d886b31c77 100644 --- a/gcc/cp/coroutines.cc +++ b/gcc/cp/coroutines.cc @@ -3913,6 +3913,7 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d) if (TREE_CODE (*stmt) == BIND_EXPR) { tree lvar; + unsigned serial = 0; for (lvar = BIND_EXPR_VARS (*stmt); lvar != NULL; lvar = DECL_CHAIN (lvar)) { @@ -3974,16 +3975,14 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d) buf = xasprintf ("%s_%u_%u", IDENTIFIER_POINTER (lvname), lvd->nest_depth, lvd->bind_indx); else - buf = xasprintf ("_D%u_%u_%u", DECL_UID (lvar), lvd->nest_depth, - lvd->bind_indx); + buf = xasprintf ("_D%u_%u_%u", lvd->nest_depth, lvd->bind_indx, + serial++); + /* TODO: Figure out if we should build a local type that has any excess alignment or size from the original decl. */ - if (buf) - { - local_var.field_id = coro_make_frame_entry (lvd->field_list, buf, - lvtype, lvd->loc); - free (buf); - } + local_var.field_id = coro_make_frame_entry (lvd->field_list, buf, + lvtype, lvd->loc); + free (buf); /* We don't walk any of the local var sub-trees, they won't contain any bind exprs. */ }
On Thu, Apr 28, 2022 at 11:22:45PM +0100, Iain Sandoe wrote: > how about the following, which uniques the names by bind scope, scope nest and then > sequence within that? That LGTM. > --- a/gcc/cp/coroutines.cc > +++ b/gcc/cp/coroutines.cc > @@ -3913,6 +3913,7 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d) > if (TREE_CODE (*stmt) == BIND_EXPR) > { > tree lvar; > + unsigned serial = 0; > for (lvar = BIND_EXPR_VARS (*stmt); lvar != NULL; > lvar = DECL_CHAIN (lvar)) > { > @@ -3974,16 +3975,14 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d) > buf = xasprintf ("%s_%u_%u", IDENTIFIER_POINTER (lvname), > lvd->nest_depth, lvd->bind_indx); > else > - buf = xasprintf ("_D%u_%u_%u", DECL_UID (lvar), lvd->nest_depth, > - lvd->bind_indx); > + buf = xasprintf ("_D%u_%u_%u", lvd->nest_depth, lvd->bind_indx, > + serial++); > + > /* TODO: Figure out if we should build a local type that has any > excess alignment or size from the original decl. */ > - if (buf) > - { > - local_var.field_id = coro_make_frame_entry (lvd->field_list, buf, > - lvtype, lvd->loc); > - free (buf); > - } > + local_var.field_id = coro_make_frame_entry (lvd->field_list, buf, > + lvtype, lvd->loc); > + free (buf); > /* We don't walk any of the local var sub-trees, they won't contain > any bind exprs. */ > } > > > Jakub
diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc index 551ddc9cc41..2e393b2cddc 100644 --- a/gcc/cp/coroutines.cc +++ b/gcc/cp/coroutines.cc @@ -3973,6 +3973,9 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d) else if (lvname != NULL_TREE) buf = xasprintf ("%s_%u_%u", IDENTIFIER_POINTER (lvname), lvd->nest_depth, lvd->bind_indx); + else + buf = xasprintf ("_D%u_%u_%u", DECL_UID (lvar), lvd->nest_depth, + lvd->bind_indx); /* TODO: Figure out if we should build a local type that has any excess alignment or size from the original decl. */ if (buf)