From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 43135 invoked by alias); 19 Nov 2019 18:40:14 -0000 Mailing-List: contact libstdc++-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libstdc++-owner@gcc.gnu.org Received: (qmail 43118 invoked by uid 89); 19 Nov 2019 18:40:14 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-6.9 required=5.0 tests=BAYES_00,FREEMAIL_FROM,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy=funky, !si X-HELO: mail-qt1-f193.google.com Received: from mail-qt1-f193.google.com (HELO mail-qt1-f193.google.com) (209.85.160.193) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 19 Nov 2019 18:40:12 +0000 Received: by mail-qt1-f193.google.com with SMTP id j5so24333762qtn.10; Tue, 19 Nov 2019 10:40:11 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=sender:subject:to:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=r5C1pjBWCtmyFWfov0fBy+zJB+/LLp5L8rZqUWv6LOU=; b=bAT5K89CmlzSB1JZxBKC/UW7VYr6TaYNMcflw28I8Z2l20Mt+t/pGtoh3+mDqlEXDR XB6FXI+8Hqx++wlYj826aFhf9+TRZS4Yd5Y0LCSYm05SMFggR8IFlC11oYUxS20K5o1b tkzo1V8zmI/54GJUCZSNeqvtCUgcA5P8eifFDPxntTF9c/FgEMUt5JLAFVTELviK5gKR 2FM3bmazuz/XW3GXHkQtbiLrNb3HV1qwwpDXUBVK6XsVoYXdzkyVSkSLU4D5PgWXTsbX K/yk3JLpfr+0YAWZ74KVCaUfNul1HWzIaaw4XCGzAqP9PEA1vXpKc6mDOWH3S318ql6d RJyw== Return-Path: Received: from ?IPv6:2620:10d:c0a3:1407:3060:40bc:5741:31b7? ([2620:10d:c091:500::3:59f7]) by smtp.googlemail.com with ESMTPSA id f21sm10406013qkl.34.2019.11.19.10.40.08 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 19 Nov 2019 10:40:09 -0800 (PST) Subject: Re: [C++ coroutines 3/6] Front end parsing and transforms. To: Iain Sandoe , GCC Patches , libstdc++ References: <285E6AA6-17E6-4E7F-9F37-852707896DA1@sandoe.co.uk> From: Nathan Sidwell Message-ID: Date: Tue, 19 Nov 2019 18:40:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.1.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2019-11/txt/msg00084.txt.bz2 On 11/17/19 5:25 AM, Iain Sandoe wrote: > +++ b/gcc/cp/coroutines.cc > +/* ================= Morph and Expand. ================= > +/* Helpers for label creation. */ > +static tree > +create_anon_label_with_ctx (location_t loc, tree ctx) > +{ > + tree lab = build_decl (loc, LABEL_DECL, NULL_TREE, void_type_node); > + > + DECL_ARTIFICIAL (lab) = 1; > + DECL_IGNORED_P (lab) = 1; We can use true for such boolean flags now, your call. > +/* We mark our named labels as used, because we want to keep them in place > + during development. FIXME: Remove this before integration. */ FIXME? > +struct __proxy_replace > +{ > + tree from, to; > +}; std::pair ? > +/* If this is a coreturn statement (or one wrapped in a cleanup) then > + return the list of statements to replace it. */ > +static tree space between comment and function -- here and elsewhere. > + /* p.return_void and p.return_value are probably void, but it's not > + clear if that's intended to be a guarantee. CHECKME. */ CHECKME? > + /* We might have a single co_return statement, in which case, we do > + have to replace it with a list. */ 'do have to' reads weirdly -> 'have to' or 'do not have to' ? > +static tree > +co_await_expander (tree *stmt, int * /*do_subtree*/, void *d) > +{ > + r = build2_loc (loc, INIT_EXPR, TREE_TYPE (sv_handle), sv_handle, > + suspend); > + append_to_statement_list (r, &body_list); > + tree resume > + = lookup_member (TREE_TYPE (sv_handle), get_identifier ("resume"), 1, 0, > + tf_warning_or_error); > + resume = build_new_method_call (sv_handle, resume, NULL, NULL_TREE, > + LOOKUP_NORMAL, NULL, tf_warning_or_error); > + resume = coro_build_cvt_void_expr_stmt (resume, loc); Do we have a way of forcing this to be a tail call? Should comment and/or TODO: > + append_to_statement_list (resume, &body_list); > + } // comments approaching ... > + add_stmt (r); // case 0: > + // Implement the suspend, a scope exit without clean ups. > + r = build_call_expr_internal_loc (loc, IFN_CO_SUSPN, void_type_node, 1, susp); > + r = coro_build_cvt_void_expr_stmt (r, loc); // danger zone over :) [there are others scattered around though] > +/* When we built the await expressions, we didn't know the coro frame > + layout, therefore no idea where to find the promise or where to put > + the awaitables. Now we know these things, fill them in. */ > +static tree > +transform_await_expr (tree await_expr, struct __await_xform_data *xform) > +{ > + struct suspend_point_info *si = suspend_points->get (await_expr); > + location_t loc = EXPR_LOCATION (await_expr); > + if (!si) > + { > + error_at (loc, "no suspend point info for %qD", await_expr); that looks implementory speak -- is it an ICE? > + /* FIXME: determine if it's better to walk the co_await several times with > + a quick test, or once with a more complex test. */ Probably can simply be an 'i'm not sure, I went with ... ' comment? > + /* Update the block associated with the outer scope of the orig fn. */ > + tree first = expr_first (fnbody); > + if (first && TREE_CODE (first) == BIND_EXPR) > + { > + /* We will discard this, since it's connected to the original scope > + nest... ??? CHECKME, this might be overly cautious. */ ? > + tree block = BIND_EXPR_BLOCK (first); > + if (block) // For this to be missing is probably a bug. gcc_assert? > + { > + gcc_assert (BLOCK_SUPERCONTEXT (block) == NULL_TREE); > + gcc_assert (BLOCK_CHAIN (block) == NULL_TREE); > + BLOCK_SUPERCONTEXT (block) = top_block; > + BLOCK_SUBBLOCKS (top_block) = block; > + } > + } > + > + add_stmt (actor_bind); > + tree actor_body = push_stmt_list (); > + > + /* FIXME: this is development marker, remove later. */ FIXME > + tree return_void = NULL_TREE; > + tree rvm > + = lookup_promise_member (orig, "return_void", loc, false /*musthave*/); /*musthave=*/false I think there are other similar cases > + /* Get a reference to the final suspend var in the frame. */ > + transform_await_expr (final_await, &xform); > + r = coro_build_expr_stmt (final_await, loc); > + add_stmt (r); > + > + /* now do the tail of the function. */ now->Now > + /* Here deallocate the frame (if we allocated it), which we will have at > + present. */ sentence is confusing. reword? > +/* Helper that returns an identifier for an appended extension to the > + current un-mangled function name. */ > +static tree > +get_fn_local_identifier (tree orig, const char *append) > +{ > + /* Figure out the bits we need to generate names for the outlined things > + For consistency, this needs to behave the same way as > + ASM_FORMAT_PRIVATE_NAME does. */ pity we don't have a generic helper already. > + char *an; > + if (DECL_ASSEMBLER_NAME (orig)) > + an = ACONCAT ((IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (orig)), sep, append, > + (char *) 0)); > + else if (DECL_USE_TEMPLATE (orig) && DECL_TEMPLATE_INFO (orig) > + && DECL_TI_ARGS (orig)) This seems funky. why do we care about templatedness? > + { > + tree tpl_args = DECL_TI_ARGS (orig); > + an = ACONCAT ((pfx, IDENTIFIER_POINTER (nm), (char *) 0)); > + for (int i = 0; i < TREE_VEC_LENGTH (tpl_args); ++i) > + { > + tree typ = DECL_NAME (TYPE_NAME (TREE_VEC_ELT (tpl_args, i))); > + an = ACONCAT ((an, sep, IDENTIFIER_POINTER (typ), (char *) 0)); > + } > + an = ACONCAT ((an, sep, append, (char *) 0)); > + } > + else how can we get here? > + an = ACONCAT ((pfx, IDENTIFIER_POINTER (nm), sep, append, (char *) 0)); > > +static bool > +register_await_info (tree await_expr, tree aw_type, tree aw_nam, tree susp_type, > + tree susp_handle_nam) > +{ > + if (seen) > + { > + error_at (EXPR_LOCATION (await_expr), "duplicate info for %qE", > + await_expr); ICE? > + > +/* Helper to return the type of an awaiter's await_suspend() method. > + We start with the result of the build method call, which will be either > + a call expression (void, bool) or a target expressions (handle). */ > +static tree > +get_await_suspend_return_type (tree aw_expr) > +{ > + tree susp_fn = TREE_VEC_ELT (TREE_OPERAND (aw_expr, 3), 1); > + debug_tree (susp_fn); ? > + return TREE_TYPE (susp_fn); > +} > + > +/* Walk the sub-tree looking for call expressions that both capture > + references and have compiler-temporaries as parms. */ > +static tree > +captures_temporary (tree *stmt, int *do_subtree, void *d) > +{ > + else > + /* This wouldn't be broken, and we assume no need to replace it > + but (ISTM) unexpected. */ > + fprintf (stderr, "target expr init var real?\n"); ? > + } > + else > + { > + debug_tree (parm); ? > + if (existed) > + { > + fprintf (stderr, "duplicate lvar: "); > + debug_tree (lvar); > + gcc_checking_assert (!existed); ? > + /* TODO: Figure out if we should build a local type that has any > + excess alignment or size from the original decl. */ ? > +bool > +morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer) > +{ > + if (!orig || TREE_CODE (orig) != FUNCTION_DECL) > + return false; assert? > + gcc_assert (orig == current_function_decl); If these have to be the same, why not just use the latter? > + /* 2. Types we need to define or look up. */ > + > + suspend_points = hash_map::create_ggc (11); If this is ggc allocated, then shouldn't suspend_points be marked GTY? However, IIUC this is only live during the processing of this function, so no GC will occur. But take care with the early returns below in that case. > + /* FIXME: this is development marker, remove later. */ that time has come :) > + if (block) // missing block is probably an error. assert? This looks similar to earlier code -- helper routine? > + /* ==== start to build the final functions. funky format > + We push_deferring_access_checks to avoid these routines being seen as > + nested by the middle end, we are doing the outlining here. */ a weird side-effect of access pushing, but whatever. > + > + push_deferring_access_checks (dk_no_check); It probably makes sense for someone more familiar with the expanders than me to take a second look at the expander chunk of the file. nathan -- Nathan Sidwell