From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id 1DF03385840F for ; Fri, 3 Sep 2021 14:08:00 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 1DF03385840F Received: from mail-qt1-f199.google.com (mail-qt1-f199.google.com [209.85.160.199]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-39-NK91G7raN9KB-cBk2c3ggw-1; Fri, 03 Sep 2021 10:07:54 -0400 X-MC-Unique: NK91G7raN9KB-cBk2c3ggw-1 Received: by mail-qt1-f199.google.com with SMTP id l22-20020a05622a175600b0029d63a970f6so5204688qtk.23 for ; Fri, 03 Sep 2021 07:07:54 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=wdv18Jw0f8kyvSX1ONW7aipW90+cvIBmnR6ebESzqCM=; b=Ko0bwVZ+uKVPNA3XTTzgySJEQ+RNajkysbbTOROS+ABxpc6gkBFXmUdfsrhAVcHNFs hRitCBeFxKqBwPZDNcvs1gx+LHhvKgdD52rrtGgiPDqT2ZQ6xKr+uLxgyFXOHk7tCncY S3fMqBAn90Rve/bvNf8zQUYv9Afk/Z+WL/8c7XeaSgDJ+R4OirnBUZzvJR9TYQOewgpD 4jfvrbqbKerYtsrYxUBTyQs4PtadMLjhqYPGEjUbJMIAXq8KNQZXUnvdv63ksPodrLG+ 7VozsuK4rBjtHdAxd/0IcYDI2sfP51uzRF5kUv1Tvrs1+Kh+eS5wf1/f8oKyJPp5moOX Lw9Q== X-Gm-Message-State: AOAM531Vq7DXYRepF2hNd6ilf1R5LjxtiXg/Ac1rTkjfKGBfPeriZvnW 9lesRbvMQUxLIvM6i/y8jfKBn0rqrqX/4j0eVAINmvBMi+oTjtJGyfZfEpKP3cvqlSKtfDc0UAE /pkQgIt+MfGXaTyPmWSsf/HND1UkpmwV0OYsFVEuGuhIfT9OlL2GxWdng1lEilpWa+Q== X-Received: by 2002:ac8:66da:: with SMTP id m26mr3915699qtp.273.1630678073598; Fri, 03 Sep 2021 07:07:53 -0700 (PDT) X-Google-Smtp-Source: ABdhPJw/WARJeGOnHEBwTIks8i9/4HIjU9Rd0iPRjo/rf+ZFU5wHkx+yxGxzsQK+VPAXzkgNj5cnzQ== X-Received: by 2002:ac8:66da:: with SMTP id m26mr3915647qtp.273.1630678073012; Fri, 03 Sep 2021 07:07:53 -0700 (PDT) Received: from [192.168.1.149] (130-44-159-43.s11817.c3-0.arl-cbr1.sbo-arl.ma.cable.rcncustomer.com. [130.44.159.43]) by smtp.gmail.com with ESMTPSA id c1sm3210662qtj.36.2021.09.03.07.07.51 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 03 Sep 2021 07:07:52 -0700 (PDT) Subject: Re: [PATCH 7/8] coroutines: Make proxy vars for the function arg copies. To: Iain Sandoe , GCC Patches References: <4C7B8763-1292-450A-96FE-A56B66926540@sandoe.co.uk> <3E415FEA-27D9-4C04-BFEE-002F67EED9F6@sandoe.co.uk> <7B788329-6FAD-4EE0-B562-B39D559FE5EE@sandoe.co.uk> <036890C2-4D5C-4E7A-B18F-037C740C3283@sandoe.co.uk> <5F6F1AC7-F686-47B1-91B0-E881AB20B5A7@sandoe.co.uk> From: Jason Merrill Message-ID: Date: Fri, 3 Sep 2021 10:07:51 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.13.0 MIME-Version: 1.0 In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-13.4 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, 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: Fri, 03 Sep 2021 14:08:02 -0000 On 9/1/21 6:56 AM, Iain Sandoe wrote: > > This adds top level proxy variables for the coroutine frame > copies of the original function args. These are then available > in the debugger to refer to the frame copies. We rewrite the > function body to use the copies, since the original parms will > no longer be in scope when the coroutine is running. > > Signed-off-by: Iain Sandoe > > gcc/cp/ChangeLog: > > * coroutines.cc (struct param_info): Add copy_var. > (build_actor_fn): Use simplified param references. > (register_param_uses): Likewise. > (rewrite_param_uses): Likewise. > (analyze_fn_parms): New function. > (coro_rewrite_function_body): Add proxies for the fn > parameters to the outer bind scope of the rewritten code. > (morph_fn_to_coro): Use simplified version of param ref. > --- > gcc/cp/coroutines.cc | 247 ++++++++++++++++++++----------------------- > 1 file changed, 117 insertions(+), 130 deletions(-) > > diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc > index aacf352f1c9..395e5c488e5 100644 > --- a/gcc/cp/coroutines.cc > +++ b/gcc/cp/coroutines.cc > @@ -1964,6 +1964,7 @@ transform_await_wrapper (tree *stmt, int *do_subtree, void *d) > struct param_info > { > tree field_id; /* The name of the copy in the coroutine frame. */ > + tree copy_var; /* The local var proxy for the frame copy. */ > vec *body_uses; /* Worklist of uses, void if there are none. */ > tree frame_type; /* The type used to represent this parm in the frame. */ > tree orig_type; /* The original type of the parm (not as passed). */ > @@ -2169,36 +2170,6 @@ build_actor_fn (location_t loc, tree coro_frame_type, tree actor, tree fnbody, > /* Declare the continuation handle. */ > add_decl_expr (continuation); > > - /* Re-write param references in the body, no code should be generated > - here. */ > - if (DECL_ARGUMENTS (orig)) > - { > - tree arg; > - for (arg = DECL_ARGUMENTS (orig); arg != NULL; arg = DECL_CHAIN (arg)) > - { > - bool existed; > - param_info &parm = param_uses->get_or_insert (arg, &existed); > - if (!parm.body_uses) > - continue; /* Wasn't used in the original function body. */ > - > - tree fld_ref = lookup_member (coro_frame_type, parm.field_id, > - /*protect=*/1, /*want_type=*/0, > - tf_warning_or_error); > - tree fld_idx = build3_loc (loc, COMPONENT_REF, parm.frame_type, > - actor_frame, fld_ref, NULL_TREE); > - > - /* We keep these in the frame as a regular pointer, so convert that > - back to the type expected. */ > - if (parm.pt_ref) > - fld_idx = build1_loc (loc, CONVERT_EXPR, TREE_TYPE (arg), fld_idx); > - > - int i; > - tree *puse; > - FOR_EACH_VEC_ELT (*parm.body_uses, i, puse) > - *puse = fld_idx; > - } > - } > - > /* Re-write local vars, similarly. */ > local_vars_transform xform_vars_data > = {actor, actor_frame, coro_frame_type, loc, local_var_uses}; > @@ -3772,11 +3743,11 @@ struct param_frame_data > bool param_seen; > }; > > -/* A tree-walk callback that records the use of parameters (to allow for > - optimizations where handling unused parameters may be omitted). */ > +/* A tree walk callback that rewrites each parm use to the local variable > + that represents its copy in the frame. */ > > static tree > -register_param_uses (tree *stmt, int *do_subtree ATTRIBUTE_UNUSED, void *d) > +rewrite_param_uses (tree *stmt, int *do_subtree ATTRIBUTE_UNUSED, void *d) > { > param_frame_data *data = (param_frame_data *) d; > > @@ -3784,7 +3755,7 @@ register_param_uses (tree *stmt, int *do_subtree ATTRIBUTE_UNUSED, void *d) > if (TREE_CODE (*stmt) == VAR_DECL && DECL_HAS_VALUE_EXPR_P (*stmt)) > { > tree t = DECL_VALUE_EXPR (*stmt); > - return cp_walk_tree (&t, register_param_uses, d, NULL); > + return cp_walk_tree (&t, rewrite_param_uses, d, NULL); > } > > if (TREE_CODE (*stmt) != PARM_DECL) > @@ -3798,16 +3769,87 @@ register_param_uses (tree *stmt, int *do_subtree ATTRIBUTE_UNUSED, void *d) > param_info &parm = data->param_uses->get_or_insert (*stmt, &existed); > gcc_checking_assert (existed); > > - if (!parm.body_uses) > + *stmt = parm.copy_var; > + return NULL_TREE; > +} > + > +/* Build up a set of info that determines how each param copy will be > + handled. */ > + > +static hash_map *analyze_fn_parms (tree orig) Function name should be on a new line. > +{ > + if (!DECL_ARGUMENTS (orig)) > + return NULL; > + > + hash_map *param_uses = new hash_map; > + > + /* Build a hash map with an entry for each param. > + The key is the param tree. > + Then we have an entry for the frame field name. > + Then a cache for the field ref when we come to use it. > + Then a tree list of the uses. > + The second two entries start out empty - and only get populated > + when we see uses. */ > + bool lambda_p = LAMBDA_FUNCTION_P (orig); > + > + unsigned no_name_parm = 0; > + for (tree arg = DECL_ARGUMENTS (orig); arg != NULL; arg = DECL_CHAIN (arg)) > { > - vec_alloc (parm.body_uses, 4); > - parm.body_uses->quick_push (stmt); > - data->param_seen = true; > + bool existed; > + param_info &parm = param_uses->get_or_insert (arg, &existed); > + gcc_checking_assert (!existed); > + parm.body_uses = NULL; > + tree actual_type = TREE_TYPE (arg); > + actual_type = complete_type_or_else (actual_type, orig); > + if (actual_type == NULL_TREE) > + actual_type = error_mark_node; > + parm.orig_type = actual_type; > + parm.by_ref = parm.pt_ref = parm.rv_ref = false; > + if (TREE_CODE (actual_type) == REFERENCE_TYPE) > + { > + /* If the user passes by reference, then we will save the > + pointer to the original. As noted in > + [dcl.fct.def.coroutine] / 13, if the lifetime of the > + referenced item ends and then the coroutine is resumed, > + we have UB; well, the user asked for it. */ > + if (TYPE_REF_IS_RVALUE (actual_type)) > + parm.rv_ref = true; > + else > + parm.pt_ref = true; > + } > + else if (TYPE_REF_P (DECL_ARG_TYPE (arg))) > + parm.by_ref = true; > + > + parm.frame_type = actual_type; > + > + parm.this_ptr = is_this_parameter (arg); > + parm.lambda_cobj = lambda_p && DECL_NAME (arg) == closure_identifier; > + > + tree name = DECL_NAME (arg); > + if (!name) > + { > + char *buf = xasprintf ("_Coro_unnamed_parm_%d", no_name_parm++); > + name = get_identifier (buf); > + free (buf); > + } > + parm.field_id = name; > + > + if (TYPE_HAS_NONTRIVIAL_DESTRUCTOR (parm.frame_type)) > + { > + char *buf = xasprintf ("_Coro_%s_live", IDENTIFIER_POINTER (name)); > + parm.guard_var = build_lang_decl (VAR_DECL, get_identifier (buf), > + boolean_type_node); > + free (buf); > + DECL_ARTIFICIAL (parm.guard_var) = true; > + DECL_CONTEXT (parm.guard_var) = orig; > + DECL_INITIAL (parm.guard_var) = boolean_false_node; > + parm.trivial_dtor = false; > + } > + else > + parm.trivial_dtor = true; > } > - else > - parm.body_uses->safe_push (stmt); > > - return NULL_TREE; > + return param_uses; > } > > /* Small helper for the repetitive task of adding a new field to the coro > @@ -3991,6 +4033,7 @@ coro_build_actor_or_destroy_function (tree orig, tree fn_type, > > static tree > coro_rewrite_function_body (location_t fn_start, tree fnbody, tree orig, > + hash_map *param_uses, > tree resume_fn_ptr_type, > tree& resume_idx_var, tree& fs_label) > { > @@ -4074,6 +4117,36 @@ coro_rewrite_function_body (location_t fn_start, tree fnbody, tree orig, > var_list = var; > add_decl_expr (var); > > + /* If we have function parms, then these will be copied to the coroutine > + frame. Create a local variable to point to each of them so that we can > + see them in the debugger. */ > + > + if (param_uses) > + { > + gcc_checking_assert (DECL_ARGUMENTS (orig)); > + /* Add a local var for each parm. */ > + for (tree arg = DECL_ARGUMENTS (orig); arg != NULL; > + arg = DECL_CHAIN (arg)) > + { > + param_info *parm_i = param_uses->get (arg); > + gcc_checking_assert (parm_i); > + parm_i->copy_var > + = build_lang_decl (VAR_DECL, parm_i->field_id, TREE_TYPE (arg)); > + DECL_SOURCE_LOCATION (parm_i->copy_var) = DECL_SOURCE_LOCATION (arg); > + DECL_CONTEXT (parm_i->copy_var) = orig; > + DECL_ARTIFICIAL (parm_i->copy_var) = true; > + DECL_CHAIN (parm_i->copy_var) = var_list; > + var_list = parm_i->copy_var; > + add_decl_expr (parm_i->copy_var); Are these getting DECL_VALUE_EXPR somewhere? > + } > + > + /* Now replace all uses of the parms in the function body with the local > + vars. */ I think the following old comment still applies to how 'visited' is used, and should be adapted here as well: >> - /* We want to record every instance of param's use, so don't include >> - a 'visited' hash_set on the tree walk, but only record a containing >> - expression once. */ > + hash_set visited; > + param_frame_data param_data = {NULL, param_uses, > + &visited, fn_start, false}; > + cp_walk_tree (&fnbody, rewrite_param_uses, ¶m_data, NULL); > + } > > /* We create a resume index, this is initialized in the ramp. */ > resume_idx_var > @@ -4343,7 +4416,9 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer) > > tree resume_idx_var = NULL_TREE; > tree fs_label = NULL_TREE; > - fnbody = coro_rewrite_function_body (fn_start, fnbody, orig, > + hash_map *param_uses = analyze_fn_parms (orig); > + > + fnbody = coro_rewrite_function_body (fn_start, fnbody, orig, param_uses, > act_des_fn_ptr, > resume_idx_var, fs_label); > /* Build our dummy coro frame layout. */ > @@ -4352,94 +4427,6 @@ morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer) > /* The fields for the coro frame. */ > tree field_list = NULL_TREE; > > - /* Now add in fields for function params (if there are any). > - We do not attempt elision of copies at this stage, we do analyze the > - uses and build worklists to replace those when the state machine is > - lowered. */ > - > - hash_map *param_uses = NULL; > - if (DECL_ARGUMENTS (orig)) > - { > - /* Build a hash map with an entry for each param. > - The key is the param tree. > - Then we have an entry for the frame field name. > - Then a cache for the field ref when we come to use it. > - Then a tree list of the uses. > - The second two entries start out empty - and only get populated > - when we see uses. */ > - param_uses = new hash_map; > - bool lambda_p = LAMBDA_FUNCTION_P (orig); > - > - unsigned no_name_parm = 0; > - for (tree arg = DECL_ARGUMENTS (orig); arg != NULL; > - arg = DECL_CHAIN (arg)) > - { > - bool existed; > - param_info &parm = param_uses->get_or_insert (arg, &existed); > - gcc_checking_assert (!existed); > - parm.body_uses = NULL; > - tree actual_type = TREE_TYPE (arg); > - actual_type = complete_type_or_else (actual_type, orig); > - if (actual_type == NULL_TREE) > - actual_type = error_mark_node; > - parm.orig_type = actual_type; > - parm.by_ref = parm.pt_ref = parm.rv_ref = false; > - if (TREE_CODE (actual_type) == REFERENCE_TYPE) > - { > - /* If the user passes by reference, then we will save the > - pointer to the original. As noted in > - [dcl.fct.def.coroutine] / 13, if the lifetime of the > - referenced item ends and then the coroutine is resumed, > - we have UB; well, the user asked for it. */ > - if (TYPE_REF_IS_RVALUE (actual_type)) > - parm.rv_ref = true; > - else > - parm.pt_ref = true; > - } > - else if (TYPE_REF_P (DECL_ARG_TYPE (arg))) > - parm.by_ref = true; > - > - parm.frame_type = actual_type; > - > - parm.this_ptr = is_this_parameter (arg); > - parm.lambda_cobj = lambda_p && DECL_NAME (arg) == closure_identifier; > - > - char *buf; > - if (DECL_NAME (arg)) > - { > - tree pname = DECL_NAME (arg); > - buf = xasprintf ("_P_%s", IDENTIFIER_POINTER (pname)); > - } > - else > - buf = xasprintf ("_P_unnamed_%d", no_name_parm++); > - > - if (TYPE_HAS_NONTRIVIAL_DESTRUCTOR (parm.frame_type)) > - { > - char *gbuf = xasprintf ("%s_live", buf); > - parm.guard_var > - = build_lang_decl (VAR_DECL, get_identifier (gbuf), > - boolean_type_node); > - free (gbuf); > - DECL_ARTIFICIAL (parm.guard_var) = true; > - DECL_INITIAL (parm.guard_var) = boolean_false_node; > - parm.trivial_dtor = false; > - } > - else > - parm.trivial_dtor = true; > - parm.field_id = coro_make_frame_entry > - (&field_list, buf, actual_type, DECL_SOURCE_LOCATION (arg)); > - free (buf); > - } > - > - /* We want to record every instance of param's use, so don't include > - a 'visited' hash_set on the tree walk, but only record a containing > - expression once. */ > - hash_set visited; > - param_frame_data param_data > - = {&field_list, param_uses, &visited, fn_start, false}; > - cp_walk_tree (&fnbody, register_param_uses, ¶m_data, NULL); > - } > - > /* We need to know, and inspect, each suspend point in the function > in several places. It's convenient to place this map out of line > since it's used from tree walk callbacks. */ >