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?
Here are my final notes from this pass over the changes. If you could
send an updated patch to the list, it would be appreciated. I may have
more comments now that I've looked over everything and have a slightly
better understanding of the overall structure.
I'm not a C++ guy, but are you going to have to do anything special with
lambdas? Such as in cilk_set_spawn_marker?
I'm going to assume you don't need to do anything for the C++ specific
decls in copy_decl_for_cilk. Note I didn't look at any of the C++
specific files, so if you're handling it there, that's fine with me.
I didn't look at the tests closely. How thorough are those tests,
particularly for front-end issues? The reason I ask is those tests will
help ensure that folks don't break the cilk+ support as often as they
might otherwise. Basically they cover for the lack of knowledge about
the cilk+ implementation by providing developers a heads-up that they
broke something. Are there any internal testsuites Intel could
contribute to help beef up testing?
Can you #undef the helper macros SUBTREE, MODIFIED, INITIALIZED that are
defined in extract_free_variables? A nit, but those kind of defines can
certainly surprised folks. Actually, unless there's a compelling
reason, why not just go ahead and make the calls to
extract_free_variables explicit and drop the macros completely?
In all, I didn't see anything that made me say "wait, this is a huge
issue we need to address". Presumably you and Aldy worked through those
before I got involved ;-)
jeff
