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.
builtins.c:
+ if (flag_enable_cilkplus && (!strcmp (name, "__cilkrts_detach")
+ || !strcmp (name, "__cilkrts_pop_frame")))
Formatting nit.
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?
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"?
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.
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.
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?
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. */
+
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.
Can you take a look at calls.c::special_function_p and determine if we
need to do something special for spawn here?
More tomorrow...
jeff
