This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C


[rth, law, jakub: Your input required throughout...please.]

More review stuff...

Overall, I must say, I'm not a big fan of the super early expansion you're doing right after parsing. I mean, you leave CILK_SPAWN and CILK_SYNC keywords as is (in tree form until gimplification) but there's this awful lot of expansion that goes on parallel to that. For instance, for this code:

extern void testing();
extern void ending();

foo(){
    _Cilk_spawn bar();
    testing();
    _Cilk_sync;
    ending();
}

...right after parsing you already generate:

{
  __cilkrts_enter_frame_1 (&D.1776);
  try
    {
      _Cilk_spawn bar ();	// keywords as trees, fine
      testing ();
      _Cilk_sync;;		// keywords as trees, fine
      ending ();
    }
  finally			// but all this try/finally
				// support stuff too early??
    {
      D.1776.pedigree = D.1776.worker->pedigree;
      if ((D.1776.flags & 2) != 0)
        {
          __cilkrts_save_fp_ctrl_state (&D.1776);
          if (__builtin_setjmp (&D.1776.ctx) == 0)
            {
              __cilkrts_sync (&D.1776);
            }
          else
            {
              if ((D.1776.flags & 16) != 0)
                {
                  __cilkrts_rethrow (&D.1776);
                }
            }
        }
      D.1776.worker->pedigree.rank = D.1776.worker->pedigree.rank + 1;
      D.1776.worker->current_stack_frame = D.1776.call_parent;
      __cilkrts_pop_frame (&D.1776);
      if (D.1776.flags != 16777216)
        {
          __cilkrts_leave_frame (&D.1776);
        }
    }
}

You seem to be hijacking finish_function(), to generate all this try/finally and builtin stuff here:

diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c
index f7ae648..ffd62c6 100644
--- a/gcc/c/c-decl.c
+++ b/gcc/c/c-decl.c
@@ -8380,6 +8380,12 @@ finish_function (void)
   /* Tie off the statement tree for this function.  */
   DECL_SAVED_TREE (fndecl) = pop_stmt_list (DECL_SAVED_TREE (fndecl));

+  /* IF the function has _Cilk_spawn in front of a function call inside it
+     i.e. it is a spawning function, then add the appropriate Cilk plus
+     functions inside.  */
+  if (flag_enable_cilkplus && cfun->calls_cilk_spawn == 1)
+    cfun->cilk_frame_decl = insert_cilk_frame (fndecl);

I would've preferred to leave all expansion until *at least* gimplification. Although, looking at how OMP works, expansion happens even further down after the CFG has been built and EH has been handled. Seeing that _Cilk_spawn and _Cilk_sync heavily alter the flow (and possibly exceptions) of the program, I would guess that you need to follow a similar path with these two Cilk keywords.

I don't think this is a blocker, especially since most everything seems to be contained in Cilk specific files, but I would like to get some feedback from Jeff/Richard/Jakub, and/or some other more globally savvy people :). Perhaps this could be done as a follow-up patch, if necessary. Having said that, at least expansion in finish_function() feels weird.

+tree
+c_build_cilk_spawn (location_t loc, tree call)
+{
+  if (!cilkplus_set_spawn_marker (loc, call))
+    return error_mark_node;
+  tree spawn_stmt = build1 (CILK_SPAWN_STMT, TREE_TYPE (call), call);

Do you need a TREE_TYPE here at all?  Is it used anywhere?

+/* Marks CALL, a CALL_EXPR, as a spawned function call.  */
+
+tree
+c_build_cilk_spawn (location_t loc, tree call)
+{
+  if (!cilkplus_set_spawn_marker (loc, call))
+    return error_mark_node;

Can you inline cilkplus_set_spawn_marker? It's not used anywhere else and it's relatively small. If you need it for C++, perhaps you can make c_build_cilk_spawn() generic enough to be used for both FE's?

+/* Helper function for walk_tree.  If *TP is a CILK_SPAWN_STMT, then unwrap
+   this "wrapper" and  *WALK_SUBTREES is set to 0.  The function returns
+   NULL_TREE regardless.  */
+
+static tree
+unwrap_cilk_sync_stmt (tree *tp, int *walk_subtrees, void *)
+{
+  if (TREE_CODE (*tp) == CILK_SPAWN_STMT)
+    {
+      *tp = CILK_SPAWN_FN (*tp);
+      *walk_subtrees = 0;
+    }
+  return NULL_TREE;
+}

Why is this called unwrap_cilk_sync_stmt when it is unwrapping a spawn?

+    case CILK_SYNC_STMT:
+      pp_string (buffer, "_Cilk_sync;");
+      break;

Drop the semi-colon. The end of dump_generic_node() should add the semi-colon if it's a statement. I bet you're probably getting two semi-colons at the end of _Cilk_sync in the dump.

+/* Returns a wrapper function for a _Cilk_spawn.  */
+
+static tree
+build_cilk_wrapper (tree exp, tree *args_out)

This name is confusing. The world "build" is usually used to denote front-end building of trees, not gimplification. Seeing that build_cilk_wrapper() is only called from gimplify_cilk_spawn(), it's a bit confusing.

+/* This function will expand a cilk_sync call.  */
+
+static tree
+build_cilk_sync (void)
+{
+  tree frame = cfun->cilk_frame_decl;

Similarly with this as well, which is mostly a helper for gimplify_cilk_sync(). And for that matter, it can be confused with c_build_cilk_sync() which does something completely different.

I do see that you also call build_cilk_sync() from build_cilk_function_exit() here:

+/* This function will output the exit conditions for a spawn call.  */
+
+tree
+build_cilk_function_exit (tree frame, bool detaches, bool needs_sync)
+{
+  tree sync_expr = NULL_TREE;
+
+  tree epi = alloc_stmt_list ();
+
+  if (needs_sync)
+    {
+      sync_expr = build_cilk_sync ();
+      append_to_statement_list (sync_expr, &epi);
+    }

It seems to me that you should have generated this implicit _Cilk_sync at function exit by an earlier pass. If jakub/rth/others think it is incorrect to expand so early (as I have pointed out), perhaps adding the _Cilk_sync could be done similarly to how OMP generates implicit OMP returns in the omplower stage.

For example, for something like:

foo(){
    int i;
#pragma omp for
    for (i=0; i < 10; ++i)
      bar();
}

omplower expands to:

    #pragma omp for private(i)
    for (i = 0; i <= 9; i = i + 1)
    bar ();
    #pragma omp continue (i, i)
    #pragma omp return

Anyways... let's wait to hear from the heavy hitters.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]