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,
	I have attached 2 patches - 1 for C and 1 for C++ - along with the changelogs (ChangeLog.cilkplus for C and common changes, cp-ChangeLog.cilkplus for C++ specific files) with the changes you have requested.  Answers to your questions are given below also.

	It passes all its tests and doesn't affect any other existing tests (i.e. by affect I mean fail a passing test or pass a failing test).

Thanks,

Balaji V. Iyer.

> -----Original Message-----
> From: Jeff Law [mailto:law@redhat.com]
> Sent: Monday, October 21, 2013 2:53 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?
> 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?
> 

Actually, if I am not mistaken (and Jason please feel free to correct me), by the time we hit the gimplify phase, a lambda function is sort of converted to a regular function. This is the precise reason for me to wait till gimplify phase to handle _Cilk_spawn and _Cilk_sync conversion. I have a test that will spawn a lambda function in the C++ patch.

The C++ patch pretty much just handles the front-end related stuff. It parses the keyword and then in pt.c, it recognizes the _Cilk_spawn and _Cilk_sync keyword and then passes them along to the next phase, which hits gimplify phase and then we gimplify it like we do it in C.

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

I have made effort to make sure that the testsuite will test all the known ways that our customers use _Cilk_spawn and _Cilk_sync. Same goes for the flags in testsuite also. I have also made sure I step through all (if not almost all) the error calls. Also, in C++ I have tried to have cases where it will do operator overloading, lambda function spawning, templatized functions (and their hybrid flavors).

> 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?
> 
I tried to model it sort of like walk_tree routine. I have removed those #defines and have called the function explicitly.

> 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 ;-)
> 

Yup. Aldy was great at catching a lot of issues :-).


> jeff

Attachment: diff-spawn-sync-c.txt
Description: diff-spawn-sync-c.txt

Attachment: diff-spawn-sync-c++.txt
Description: diff-spawn-sync-c++.txt

Attachment: ChangeLog.cilkplus
Description: ChangeLog.cilkplus

Attachment: cp-ChangeLog.cilkplus
Description: cp-ChangeLog.cilkplus


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