_Cilk_spawn and _Cilk_sync for C++

Hi Jason,
	I am attaching a fixed patch with answers to your questions below. The patch passes all the tests for C and C++ on my x86_64 SUSE box.

Thanks,

Balaji V. Iyer.

> -----Original Message-----
> From: Jason Merrill [mailto:jason@redhat.com]
> Sent: Monday, December 2, 2013 5:19 PM
> To: Iyer, Balaji V; gcc-patches@gcc.gnu.org
> Cc: Jeff Law
> Subject: Re: _Cilk_spawn and _Cilk_sync for C++
> 
> On 11/28/2013 11:40 AM, Iyer, Balaji V wrote:
> > Consider the following test case. I took this from the lambda_spawns.cc
> line #203.
> >
> > as you can tell, it is clobbering the lambda closure at the end of the lambda
> calling and then it is catching value of A from main2 as it is supposed to.
> 
> Yep, your patch gives a fine result for this testcase.
> 
> > What am I misunderstanding?
> 
> It just gets there for the wrong reason: remapping exactly nothing in the
> closure object happens to give the desired semantics, treating the
> temporaries in the CONSTRUCTOR as local to the spawned function and
> referring to variables from the spawning context via the nested function
> static chain.  But presumably you have all that remapping machinery there
> because doing nothing doesn't always give the desired result.  Right?
> 
> When you add CONSTRUCTOR handling to extract_free_variables, you get
> the crash in gimplify_var_or_parm_decl because you don't specifically handle
> VEC_INIT_EXPR, which needs to be handled a lot like TARGET_EXPR, and end
> up trying to pass the address of its temporary object into the spawned
> function, which doesn't work because, like a TARGET_EXPR, the temporary
> doesn't exist outside of the VEC_INIT_EXPR.  This doesn't mean that handling
> CONSTRUCTOR is wrong; leaving it out means that you aren't going to handle
> aggregate temporaries properly either.
> 
> I think it might be better for gimplify_cilk_spawn to gimplify the call
> expression first, and then do your transformation on the gimple, so you don't
> have to worry about language-specific magic.
> 
> Now, after all that I must admit that cilk_spawn could only ever see
> VEC_INIT_EXPR in the context of a lambda closure initialization, and the
> default behavior should always be correct for a lambda closure initialization,
> so I guess I'm willing to allow the magic lambda handling with a comment
> about it being a workaround.

Thanks for this! Where should I put this comment?

> 
> > +is_lambda_fn_p (tree call_exp)
> > +{
> > +  if (TREE_CODE (call_exp) != CALL_EXPR)
> > +    return false;
> > +  tree call_fn = CALL_EXPR_FN (call_exp);
> > +  if (TREE_CODE (call_fn) == ADDR_EXPR)
> > +    call_fn = TREE_OPERAND (call_fn, 0);
> 
> Use get_callee_fndecl to get the FUNCTION_DECL.  And change the name of
> the function, since it isn't testing whether the argument is itself a lambda
> function; perhaps call_to_lambda_fn_p?
> 

This is fixed.

> >         case CILK_SPAWN_STMT:
> >           gcc_assert
> >             (fn_contains_cilk_spawn_p (cfun)
> >              && lang_hooks.cilkplus.cilk_detect_spawn_and_unwrap (expr_p));
> >           if (!seen_error ())
> >             {
> >               ret = (enum gimplify_status)
> >                 lang_hooks.cilkplus.gimplify_cilk_spawn (expr_p, pre_p,
> >                                                          post_p);
> >               break;
> >             }
> >           /* If errors are seen, then just process it as a CALL_EXPR.
> > */
> 
> Please remove these langhooks and instead add handling of
> CILK_SPAWN_STMT to c_gimplify_expr and cp_gimplify_expr.

I mentioned this in a previous thread (http://gcc.gnu.org/ml/gcc-patches/2013-12/msg00244.html). 

> 
> >   lang_hooks.cilkplus.install_body_with_frame_cleanup (fndecl, stmt,
> >                                                        (void *) wd);
> 
> And instead of this langhook, declare a function in c-common.h that is
> defined by all C family front ends.
> 
> > +    stabilize_expr (orig_body, &pre_body);
> 
> Here you're pre-evaluating the entire call, rather than just the lambda
> closure object, which means none of the arguments to the call will be
> remapped.  I think you want
> 

I stabilize the call here, and then I call cilk_outline on the pre_body which will do the outlining of the arguments. Also, I don't need a specialized copy_tree_body_r for C++ to handle the lambda functions.

> CALL_EXPR_ARG (orig_body, 0)
>    = stabilize_expr (CALL_EXPR_ARG (orig_body, 0), &pre_body);
> append_to_statement_list (orig_body, &pre_body);
> 
> instead.
> 
> > +  gcc_assert (TREE_CODE (catch_list) == STATEMENT_LIST);
> 
> You don't need this, append_to_statement_list handles the list not yet being
> a list fine.

This is fixed.

> 
> > +       /* We set this here so that finish_call_expr can set lambda to a var.
> > +          if it is not done so.  */
> 
> This comment is obsolete.
> 

Fixed.

> > +       error_at (input_location, "_Cilk_sync cannot be used without enabling
> "
> > +                 "Cilk Plus");
> > +      cp_lexer_consume_token (parser->lexer);
> > +      if (parser->in_statement & IN_CILK_SPAWN)
> > +       parser->in_statement = parser->in_statement & ~IN_CILK_SPAWN;
> 
> Why are you messing with in_statement in the cilk_spawn code?
> 
This needed to catch cases like this:

_Cilk_spawn _Cilk_spawn foo () 

There could be other things between those two _Cilk_spawns   eg. '('.

But the comment above is misleading and I fixed that. Also, it should not be  = IN_CILK_SPAWN, but |= IN_CILK_SPAWN. I fixed that also.

Thanks,

Balaji V. Iyer.

> Jason

_Cilk_spawn and _Cilk_sync for C++

Commit Message

Comments

Patch