This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Hi Jeff,
	Please see my comments below:

Thanks,

Balaji V. Iyer.

> -----Original Message-----
> From: Jeff Law [mailto:law@redhat.com]
> Sent: Wednesday, October 16, 2013 5:23 PM
> To: Iyer, Balaji V; rth@redhat.com; Jason Merrill (jason@redhat.com); Aldy
> Hernandez (aldyh@redhat.com)
> Cc: gcc-patches@gcc.gnu.org
> Subject: Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and C++)
> 
> On 09/11/13 12:18, Iyer, Balaji V wrote:
> > Hello Everyone, Couple weeks back, I had submitted a patch for review
> > that will implement Cilk keywords (_Cilk_spawn and _Cilk_sync) into
> > the C compiler. I recently finished C++ implementation also. In this
> > email, I am attaching 2 patches: 1 for C (and the common parts for C
> > and C++) and 1 for C++. The C++ Changelog is labelled
> > cp-ChangeLog.cilkplus and the other one is just ChangeLog.cilkplus.
> > There isn't much changes in the C patch. Only noticeable changes would
> > be moving functions to the common parts so that C++ can use them.
> >
> > It passes all the tests and does not affect  (by affect I mean fail a
> > passing test or pass a failing one) any of the other tests in the
> > testsuite directory.
> >
> > Is this Ok for trunk?
> This is not a complete review, but obviously I am starting to look at the patch
> and figured you could start addressing issues as I find them.
>   I usually start by trying to filter out all the stuff that is fairly benign, so I'm
> looking at code in a fairly random order.
> 
> You'll also note some items are just questions I'd like you to answer and don't (at
> this time) require any code changes -- they just help me understand everything.
> 
> I'm also not looking at the C++ bits -- my familiarity with the C++ front-end is
> minimal at best and I'd probably do more harm than good reviewing that code.
> 
> 
> 
> In gcc/Makefile.in, since the original submission of your patch we have
> integrated automatic dependency generation.  As a result several hunks to
> provide dependencies for cilk.o and update dependencies for other objects are
> no longer needed.  This is true for hte various Make-lang.in files as well.
> 

Yup, I saw that and I will fix it in the next patch.

> builtins.c:
> 
> +  if (flag_enable_cilkplus && (!strcmp (name, "__cilkrts_detach")
> +                              || !strcmp (name,
> + "__cilkrts_pop_frame")))
> Formatting nit.

Ok...will fix it...

>    if (fubar
>        && (com | baz))
> 
> in cppbuiltin.c, presumably the __cilk predefine is some kind of version #?  I'm
> going to assume that __clik is the same #define that ICC sets up, correct?
> 

Yes, ICC and GCC defines __cilk as 200.

> In function.h:
> 
> +  /* This will indicate whether a function is a cilk function */
> + unsigned int is_cilk_function : 1;
> 
> Doesn't this really mean "calls into the cilk runtime"?
> 
> 

What I meant was that it is calling one of the Cilk functions that are defined in the runtime.

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

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

> 
> In gimplify.c:
> +  /* Implicit _Cilk_sync must be inserted right before any return statement
> +     if there is a _Cilk_spawn in the function.  If the user has provided a
> +     _Cilk_sync, the optimizer should remove this duplicate one.  */
> + if (fn_contains_cilk_spawn_p (cfun))
> +    {
> +      tree impl_sync = build0 (CILK_SYNC_STMT, void_type_node);
> +      gimplify_and_add (impl_sync, pre_p);
> +    }
> +
> 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.

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

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

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

> 
> More tomorrow...
> 
> jeff


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]