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<tree,tree> ?


+/* 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<tree, struct suspend_point_info>::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

Reply via email to