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

[Aldy Hernandez <aldyh@redhat.com>]

[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.