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

[Jeff Law <law at redhat dot com>]

On 10/16/13 15:49, Iyer, Balaji V wrote:
> > In ira.c:
> >
> > +       /* We need a frame pointer for all Cilk Plus functions that use
> > +         Cilk keywords.  */
> > +       || (flag_enable_cilkplus && cfun->is_cilk_function)
> > Can you explain to me a bit more why you need a frame pointer?  I'm trying to
> > determine if it's best to leave this as-is or have this code detect a property in the
> > generated code for the function.  From a modularity standpoint it seems pretty
> > gross that we have to peek at this within IRA.
>
> Cilk Runtime functions changes the stack pointer. So, frame pointer is necessary.
Nevermind -- that seems to be the location where this is detected now. 
So this is fine.

> > In a couple places I saw this comment:
> > +  /* Cilk keywords currently need to replace some variables that
> > +     ordinary nested functions do not.  */  bool remap_var_for_cilk;
> > I didn't see anywhere that explained exactly why some variables that do not
> > ordinarily need replacing need replacing when cilk is enabled.  If it's in the patch
> > somewhere, just point me to it. If not, add documentation about why these
> > variables need remapping for cilk.
>
> It is used in the cilk_outline function.
Thanks.  Presumably the comment "We don't want the private variables 
anymore" is the relevant code/comment?

> > > Does anything actually ensure we don't have multiple syncs?
>
> Well, _Cilk_sync expands to something like this:
>
> If (!sync_occurred)
> 	__cilkrts_sync()
>
> So, having multiple Cilk syncs doesn't harm, just that the then case of the if-statement will not be taken.
OK.  Thanks.

> > What's the thinking behind parsing calls to cilk_spawn as a normal call if there's
> > an error?  Referring to this code in gimplify.c:
> > +       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.
> > + */
> > +
>
> Well, if there is an error the compiler is not going to produce an executable. So, I just let the compiler go far as it can and catch all the other errors. If the error is cilk related, we have already called them out on it. Adding _Cilk_spawn specific routines would add additional complication.

I guess that's a reasonable fallback position in case of an error.

> > Meta-question, when we're not in cilk mode, should we be consuming the cilk
> > tokens?  I'm not familiar at all with our parser, so I'm not sure if we can handle
> > this gracefully.  Though I guess parsing hte token and warning/error if not in Cilk
> > mode is probably the best course of action.
>
> In the compiler, I couldn't make conditional tokens. When the parser hits a _Cilk_spawn or _Cilk_sync token, it will check if Cilk Plus is enabled or will complain. Now that I think about it in detail, I suppose it will also block if anyone wants to have a variable name called _Cilk_spawn or _Cilk_sync and not using -fcilkplus. But, they start with '_', and so I guess it is not a normal case.
Figured it was ugly at best to avoid consuming the cilk tokens when not 
in cilk mode.



> > Can you take a look at calls.c::special_function_p and determine if we need to
> > do something special for spawn here?
>
> I will look into it and let you know.
Any word on this?

jeff