Hi Jeff,
Please see my comments below:
Thanks,
Balaji V. Iyer.
> -----Original Message-----
> From: Jeff Law [mailto:l...@redhat.com]
> Sent: Wednesday, October 16, 2013 5:23 PM
> To: Iyer, Balaji V; r...@redhat.com; Jason Merrill (ja...@redhat.com); Aldy
> Hernandez (al...@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
