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:al...@redhat.com]
> Sent: Tuesday, August 20, 2013 6:04 PM
> To: Iyer, Balaji V
> Cc: r...@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.
