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

["Iyer, Balaji V" <balaji.v.iyer@intel.com>]

HI Aldy et al.,
	I would like to address the early expansion question. The reason why I did it is this way is because it was straight-forward for me to implement. I did some preliminary analysis, and it wasn't blocking any major optimization. Also, after parsing all I am really doing is to insert a couple functions. The expansion is actually done during gimplification (in gimplify_expr).

	If it is Ok with everyone, I would like to keep it as-is for now. After I finish getting all the Cilk Plus parts into trunk, I will go back and look into optimizing this. This way, I can have some time (november to feb, during stage3) to do some more analysis and see if I can come up with a better solution.

Thanks,

Balaji V. Iyer.

> -----Original Message-----
> From: Aldy Hernandez [mailto:aldyh@redhat.com]
> Sent: Tuesday, August 20, 2013 6:04 PM
> To: Iyer, Balaji V
> Cc: rth@redhat.com; Jeff Law; gcc-patches@gcc.gnu.org; Jakub Jelinek
> Subject: 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.