Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and C++)
On Mon, Oct 28, 2013 at 09:01:45PM +, Iyer, Balaji V wrote:
Thanks! I will extract and check in the Cilk_spawn and _Cilk_sync for C work.
This broke bootstrap on i686-linux, fixed thusly, committed as obvious:
2013-10-30 Jakub Jelinek ja...@redhat.com
* cilk.c (create_cilk_helper_decl): Use HOST_WIDE_INT_PRINT_DEC.
--- gcc/c-family/cilk.c.jj 2013-10-30 13:53:52.0 +0100
+++ gcc/c-family/cilk.c 2013-10-30 14:44:49.358912539 +0100
@@ -287,9 +287,9 @@ create_cilk_helper_decl (struct wrapper_
{
char name[20];
if (wd-type == CILK_BLOCK_FOR)
-sprintf (name, _cilk_for_%ld, cilk_wrapper_count++);
+sprintf (name, _cilk_for_ HOST_WIDE_INT_PRINT_DEC, cilk_wrapper_count++);
else if (wd-type == CILK_BLOCK_SPAWN)
-sprintf (name, _cilk_spn_%ld, cilk_wrapper_count++);
+sprintf (name, _cilk_spn_ HOST_WIDE_INT_PRINT_DEC, cilk_wrapper_count++);
else
gcc_unreachable ();
Jakub
Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and C++)
On 10/22/13 10:22, Iyer, Balaji V wrote: 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). A note. cilk-common seems to be a mix of tree and rtl bits. Those may need to be broken into separate files and/or moved around as part of the reorganization work that's occurring in GCC right now. So if Andrew or someone else pings you, please respond appropriately. The C (and shared) parts of this patch are OK. Jason needs to review the C++ front-end changes. If possible, I recommend installing the C (and shared) bits as well as the runtime component if they can build work w/o the C++ front-end changes, then track the C++ front-end changes separately. Jeff
RE: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and C++)
-Original Message- From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches- ow...@gcc.gnu.org] On Behalf Of Jeff Law Sent: Monday, October 28, 2013 4:24 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 10/22/13 10:22, Iyer, Balaji V wrote: 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). A note. cilk-common seems to be a mix of tree and rtl bits. Those may need to be broken into separate files and/or moved around as part of the reorganization work that's occurring in GCC right now. So if Andrew or someone else pings you, please respond appropriately. The C (and shared) parts of this patch are OK. Jason needs to review the C++ front-end changes. If possible, I recommend installing the C (and shared) bits as well as the runtime component if they can build work w/o the C++ front-end changes, then track the C++ front-end changes separately. Thanks! I will extract and check in the Cilk_spawn and _Cilk_sync for C work. -Balaji V. Iyer. Jeff
RE: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and C++)
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. Any word on this? Hi Jeff, I looked into this function and from what I can tell, it is used to mark certain functions (e.g. builtin functions) as special and thus don't do special optimizations on them like a regular function. The thing is, the spawnee (the function being spawned) can be pretty much any regular function. The compiler doesn't even touch inside the function. The compiler inserts specific Cilk function calls in the spawner and transplants the function . The only major restriction I know is that the frame pointer needs to be used and that I mark as I mentioned above. Is there anything you were thinking about that I missed? Thanks, Balaji V. Iyer. jeff
Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and C++)
On 10/23/13 13:46, Iyer, Balaji V wrote: 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. Any word on this? Hi Jeff, I looked into this function and from what I can tell, it is used to mark certain functions (e.g. builtin functions) as special and thus don't do special optimizations on them like a regular function. The thing is, the spawnee (the function being spawned) can be pretty much any regular function. The compiler doesn't even touch inside the function. The compiler inserts specific Cilk function calls in the spawner and transplants the function . The only major restriction I know is that the frame pointer needs to be used and that I mark as I mentioned above. Is there anything you were thinking about that I missed? There wasn't anything in particular I was worried about. Just a general question as to whether or not we needed to mark the spawner or spawnee as special, partiuclarly returns twice (setjmp/fork) and never returns (longjmp). Jeff
RE: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and C++)
-Original Message- From: Jeff Law [mailto:l...@redhat.com] Sent: Wednesday, October 23, 2013 3:53 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 10/23/13 13:46, Iyer, Balaji V wrote: 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. Any word on this? Hi Jeff, I looked into this function and from what I can tell, it is used to mark certain functions (e.g. builtin functions) as special and thus don't do special optimizations on them like a regular function. The thing is, the spawnee (the function being spawned) can be pretty much any regular function. The compiler doesn't even touch inside the function. The compiler inserts specific Cilk function calls in the spawner and transplants the function . The only major restriction I know is that the frame pointer needs to be used and that I mark as I mentioned above. Is there anything you were thinking about that I missed? There wasn't anything in particular I was worried about. Just a general question as to whether or not we needed to mark the spawner or spawnee as special, partiuclarly returns twice (setjmp/fork) and never returns (longjmp). I do check for those in the the spawnee using the check_outlined_calls function. Jeff
Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and C++)
On 10/16/13 15:49, Iyer, Balaji V wrote:
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.
Nevermind -- that seems to be the location where this is detected now.
So this is fine.
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.
Thanks. Presumably the comment We don't want the private variables
anymore is the relevant code/comment?
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.
OK. Thanks.
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.
I guess that's a reasonable fallback position in case of an error.
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.
Figured it was ugly at best to avoid consuming the cilk tokens when not
in cilk mode.
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.
Any word on this?
jeff
Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and C++)
On 10/18/13 15:06, Iyer, Balaji V wrote: Hi Jeff, Please see my comments below. Also, I am adding all these changes to the files as you requested in my local directory. Should I send you an updated patch along the way? I'll let you know when I've worked my way through everything. ISTM an updated patch after that would be best. I'm hoping to get through the remaining changes today, then review your replies in detail. jeff
Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and C++)
On 10/18/13 15:06, Iyer, Balaji V wrote: The main reason why I made it volatile (as expressed by the volatil bool variable) is that I want to make sure these values aren't optimized by the compiler and the value is fetched from memory on every access. I have added an explanation to the header comment. I figured that was the case; it's also what origianlly got me thinking about memory models. volatile can help with getting the right number of accesses (though there are cases where it gets in the way). But volatile is not a fence/barrier :-) Presumably users never concern themselves with the cilkrts_pedigree structure, it's entirely hidden, right? So there's no way for a user to ask for an array of these things, right? Ok, so this relates to the question about keeping all the structures in sync I note you set DECL_ALIGN to BIGGEST_ALIGNMENT, which seems excessive. Why not compute the natural alignment for that structure and use that? Similarly for the cilkrts_stack_frame, except that it seems to use PREFERRED_STACK_BOUNDARY. Is there some reason why each shouldn't be aligned on whatever boundary the target would normally align those structures? Fixed, I now let the compiler set these values Actually, thinking more about this, we've got to make sure we're compatible with ICC on this stuff. So if you need to weak the alignments to be ICC compatible, then, well, we have to do it. If (for example) the compilers differ on their notion of the structure's alignment, then you couldn't make an array of them and use it reliably across the ICC GCC implementations. You might consider some sort of interoperatibity test. Obviously the test wouldn't run if ICC wasn't installed, but having a test to catch this stuff early (if it ever happens) would be wise. These structures don't change often and whenever they do (i.e. fields get added into it) it is kind of a big deal. So far, when we have changed - once in the past 3 yrs. or so - we have allowed backward compatibility. Good to hear it's a big deal. Once there are multiple implementations in the wild relying upon them, it'll be an even bigger deal.. We've given it some thought, but have neither the manpower nor charter to port Cilk to non-Intel architectures. We've done a trial port to ARM to prove it could be done, but we ran fib (a small example) once and declared victory. We're sure that more work needs to be done here and would welcome changes to the runtime to support other architectures. In particular, most or all uses of assembly-language instructions should be replaced by compiler intrinsics and memory barriers probably need to be added for architectures that have a more relaxed memory model than the x86. Our hope is that, once Cilk Plus was added to gcc, that members of the community would help us port the runtime to other architectures. Such ports could probably start with full barriers, it'd be painful from a performance standpoint, but as a starting point, sufficient. jeff
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? 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
RE: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and C++)
Hi Jeff,
Please see my comments below. Also, I am adding all these changes to
the files as you requested in my local directory. Should I send you an updated
patch along the way?
Thanks,
Balaji V. Iyer.
-Original Message-
From: Jeff Law [mailto:l...@redhat.com]
Sent: Thursday, October 17, 2013 5:30 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.
More random comments, questions things to fix:
In cilk_common.c we have:
+/* Returns the value in structure FRAME pointed by the FIELD_NUMBER
+ (e.g. X.y).
+ FIELD_NUMBER is an index to the structure FRAME_PTR. For details
+ about these fields, refer to cilk_trees structure in cilk.h and
+ cilk_init_builtins function in this file. Returns a TREE that is
the type
+ of the field represented by FIELD_NUMBER. */
+
+tree
+cilk_dot (tree frame, int field_number, bool volatil) {
+ tree field = cilk_trees[field_number];
+ field = fold_build3 (COMPONENT_REF, TREE_TYPE (field), frame, field,
+ NULL_TREE);
+ TREE_THIS_VOLATILE (field) = volatil;
+ return field;
+}
There's no description of what the VOLATIL argument does. Similarly for
cilk_arrow. From reading other code it appears to have the usual meaning of
volatile, but it's best to be explicit.
The main reason why I made it volatile (as expressed by the volatil bool
variable) is that I want to make sure these values aren't optimized by the
compiler and the value is fetched from memory on every access. I have added an
explanation to the header comment.
In add_field:
+/* This function will add FIELD of type TYPE to a defined built-in
+ structure. */
+
+static tree
+add_field (const char *name, tree type, tree fields) {
+ tree t = get_identifier (name);
+ tree field = build_decl (BUILTINS_LOCATION, FIELD_DECL, t, type);
+ TREE_CHAIN (field) = fields;
+ return field;
+}
NAME is not documented. Nit: Just one space between the type specifier and
the variable name on this line:
Fixed both.
tree t = get_identifier (name);
Presumably add_field is called well in advance of layout_decl :-)
Yes.
install_builtin doesn't document CODE or PUBLISH in its comment header.
Fixed.
Presumably users never concern themselves with the cilkrts_pedigree structure,
it's entirely hidden, right? So there's no way for a user to ask for an
array of
these things, right?
A user can ignore pedigrees if they have no use for them (most don't), but
__cilkrts_pedigree is specifically intended for read-only access by the user.
A pedigree is a way of identifying where a parallel application is in the
Directed Acyclic Graph that can be used to represent it. Regardless of the
schedule chosen by the runtime, the pedigree will always be the same for the
same point in the calculation. The runtime creates and maintains this
structure, but does not use it internally for anything (unless the
record/replay logic is turned on). The pedigree is accessed using a public
function, __cilkrts_get_pedigree(), in the cilk_api.h header and can be used,
for example, to create deterministic parallel random number generators.
I note you set DECL_ALIGN to BIGGEST_ALIGNMENT, which seems excessive.
Why not compute the natural alignment for that structure and use that?
Similarly for the cilkrts_stack_frame, except that it seems to use
PREFERRED_STACK_BOUNDARY. Is there some reason why each shouldn't be
aligned on whatever boundary the target would normally align those structures?
Fixed, I now let the compiler set these values
For the ctx array, presumably it's some kind of context. Where does the
size
of that array come from?
Yes, it is context. Here is a link to the abi.h that describes fields in this
structure:
http://gcc.gnu.org/git/?p=gcc.git;a=blob;f=libcilkrts/include/internal/abi.h;h=8f64b1bc5df8a0dad18cc14bb4e2cb51a60433e9;hb=7977b69ea6b3bb535aa85a757aee3d11d37d7ff0
More generally is there any way we can ensure those structures are consistent
between the runtime
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.
More random comments, questions things to fix:
In cilk_common.c we have:
+/* Returns the value in structure FRAME pointed by the FIELD_NUMBER
+ (e.g. X.y).
+ FIELD_NUMBER is an index to the structure FRAME_PTR. For details
+ about these fields, refer to cilk_trees structure in cilk.h and
+ cilk_init_builtins function in this file. Returns a TREE that is
the type
+ of the field represented by FIELD_NUMBER. */
+
+tree
+cilk_dot (tree frame, int field_number, bool volatil)
+{
+ tree field = cilk_trees[field_number];
+ field = fold_build3 (COMPONENT_REF, TREE_TYPE (field), frame, field,
+ NULL_TREE);
+ TREE_THIS_VOLATILE (field) = volatil;
+ return field;
+}
There's no description of what the VOLATIL argument does. Similarly for
cilk_arrow. From reading other code it appears to have the usual
meaning of volatile, but it's best to be explicit.
In add_field:
+/* This function will add FIELD of type TYPE to a defined built-in
+ structure. */
+
+static tree
+add_field (const char *name, tree type, tree fields)
+{
+ tree t = get_identifier (name);
+ tree field = build_decl (BUILTINS_LOCATION, FIELD_DECL, t, type);
+ TREE_CHAIN (field) = fields;
+ return field;
+}
NAME is not documented. Nit: Just one space between the type specifier
and the variable name on this line:
tree t = get_identifier (name);
Presumably add_field is called well in advance of layout_decl :-)
install_builtin doesn't document CODE or PUBLISH in its comment header.
Presumably users never concern themselves with the cilkrts_pedigree
structure, it's entirely hidden, right? So there's no way for a user to
ask for an array of these things, right?
I note you set DECL_ALIGN to BIGGEST_ALIGNMENT, which seems excessive.
Why not compute the natural alignment for that structure and use that?
Similarly for the cilkrts_stack_frame, except that it seems to use
PREFERRED_STACK_BOUNDARY. Is there some reason why each shouldn't be
aligned on whatever boundary the target would normally align those
structures?
For the ctx array, presumably it's some kind of context. Where does
the size of that array come from?
More generally is there any way we can ensure those structures are
consistent between the runtime and the compiler? How often have those
structures changed over time and how did y'all deal with the changes?
Presumably get_frame_arg is only used on calls into the cilk runtime,
right and are all generated by GCC, and should always have the right
number of arguments, should always be compatible, etc?!? If so, then
those sanity checks should probably be asserts. Else it seems fine.
Comment for cilk_detach indicates it returns const0_rtx, but signature
has void for the return type.
I'm assuming the code in expand_cilk_sync actually implements the
pseudocode. I didn't verify every hunk of tree you built.
I'd been wondering about concurrent access to various structures in
here. x86 has a fairly easy-to-use memory model. Has any thought been
given to what would need to change to support the weaker memory models
such as are found on PPC?
More tomorrow I hope.
jeff
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.
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
[PING]RE: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and C++)
Hello, Has anyone got a chance to look into this patch? Thanks, Balaji V. Iyer. -Original Message- From: Iyer, Balaji V Sent: Tuesday, September 17, 2013 10:51 AM To: r...@redhat.com; Jason Merrill (ja...@redhat.com); 'Jeff Law'; 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++) Hello, Has anyone had a chance to look at this. The C++ part is only a week old, but the C part has been in review for ~3 weeks. I would greatly appreciate if someone could review this and approve for trunk if it is Ok for trunk. Thanks, Balaji V. Iyer. -Original Message- From: Iyer, Balaji V Sent: Wednesday, September 11, 2013 2:18 PM To: r...@redhat.com; Jason Merrill (ja...@redhat.com); Jeff Law; 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++) 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? Thanks, Balaji V. Iyer. -Original Message- From: Iyer, Balaji V Sent: Friday, August 30, 2013 1:02 PM To: gcc-patches@gcc.gnu.org Subject: FW: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C The email seem to be bouncing gcc-patches. I have gzipped my patch. Thanks, Balaji V. Iyer. -Original Message- From: Iyer, Balaji V Sent: Friday, August 30, 2013 11:42 AM To: 'Aldy Hernandez' Cc: r...@redhat.com; Jeff Law; gcc-patches@gcc.gnu.org Subject: RE: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C Hi Aldy, Attached, please find a fixed patch and the changelog entries. -Original Message- From: Aldy Hernandez [mailto:al...@redhat.com] Sent: Wednesday, August 28, 2013 2:36 PM To: Iyer, Balaji V Cc: r...@redhat.com; Jeff Law; gcc-patches@gcc.gnu.org Subject: Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C On 08/27/13 16:27, Iyer, Balaji V wrote: Hello Aldy, I went through all the emails and here are the major issues that I could gather (other than lowering the keywords after gimplification, which I am skipping since it is more of an optimization for now). Ok, for now I am fine with delaying handling all this as a gimple tuple since most of your code lives in it's only little world :). But I will go on record saying that part of the reason that you have to handle CALL_EXPR, MODIFY_EXPR, INIT_EXPR and such is because you don't have easy gimplified code to examine. Anyways, agreed, you can do this later. 1. Calling the gimplify_cilk_spawn on top of the gimplify_expr before the switch-statement could slow the compiler down 2. I need a CILK_SPAWN_STMT case in the switch statement in gimplify_expr (). 3. No test for catching the suspicious spawned function warning 4. Reasoning for expanding the 2 builtin functions in builtins.c instead of just inserting the appropriate expanded-code when I am inserting the function call. Did I miss anything else (or misunderstand anything you pointed out)? Here are my answers to those questions above and am attaching a fixed patch with the changelog entries: 1 2(partial): There are 3 places where we could have _Cilk_spawn: INIT_EXPR, CALL_EXPR and MODIFY_EXPR. INIT_EXPR and MODIFY_EXPRS are both gimplified using gimplify_modify_expr. I have moved the cilk_detect_spawn into this function. We will go into the cilk_detect_spawn if cilk plus is enabled, and if there is a cilk_frame (meaning the function has a Cilk_spawn in it) thereby reducing the number of hits into this function significantly. Inside this function, it will go into the function that has a spawned function call and then unwrap the CILK_SPAWN_STMT wrapper and returns true. This shouldn't cause a huge compilation time hit. 2. To handle CALL_EXPR (e.g. _Cilk_spawn foo (x), where
RE: [PING]RE: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and C++)
Err...I hit the send button too quickly, thus making a stupid grammatical error.. it should say Did anyone get a chance to look into this patch? Sorry about this. -Balaji V. Iyer. -Original Message- From: Iyer, Balaji V Sent: Monday, September 23, 2013 7:14 PM To: 'r...@redhat.com'; 'Jason Merrill (ja...@redhat.com)'; 'Jeff Law'; 'Aldy Hernandez (al...@redhat.com)' Cc: 'gcc-patches@gcc.gnu.org' Subject: [PING]RE: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and C++) Hello, Has anyone got a chance to look into this patch? Thanks, Balaji V. Iyer. -Original Message- From: Iyer, Balaji V Sent: Tuesday, September 17, 2013 10:51 AM To: r...@redhat.com; Jason Merrill (ja...@redhat.com); 'Jeff Law'; 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++) Hello, Has anyone had a chance to look at this. The C++ part is only a week old, but the C part has been in review for ~3 weeks. I would greatly appreciate if someone could review this and approve for trunk if it is Ok for trunk. Thanks, Balaji V. Iyer. -Original Message- From: Iyer, Balaji V Sent: Wednesday, September 11, 2013 2:18 PM To: r...@redhat.com; Jason Merrill (ja...@redhat.com); Jeff Law; 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++) 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? Thanks, Balaji V. Iyer. -Original Message- From: Iyer, Balaji V Sent: Friday, August 30, 2013 1:02 PM To: gcc-patches@gcc.gnu.org Subject: FW: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C The email seem to be bouncing gcc-patches. I have gzipped my patch. Thanks, Balaji V. Iyer. -Original Message- From: Iyer, Balaji V Sent: Friday, August 30, 2013 11:42 AM To: 'Aldy Hernandez' Cc: r...@redhat.com; Jeff Law; gcc-patches@gcc.gnu.org Subject: RE: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C Hi Aldy, Attached, please find a fixed patch and the changelog entries. -Original Message- From: Aldy Hernandez [mailto:al...@redhat.com] Sent: Wednesday, August 28, 2013 2:36 PM To: Iyer, Balaji V Cc: r...@redhat.com; Jeff Law; gcc-patches@gcc.gnu.org Subject: Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C On 08/27/13 16:27, Iyer, Balaji V wrote: Hello Aldy, I went through all the emails and here are the major issues that I could gather (other than lowering the keywords after gimplification, which I am skipping since it is more of an optimization for now). Ok, for now I am fine with delaying handling all this as a gimple tuple since most of your code lives in it's only little world :). But I will go on record saying that part of the reason that you have to handle CALL_EXPR, MODIFY_EXPR, INIT_EXPR and such is because you don't have easy gimplified code to examine. Anyways, agreed, you can do this later. 1. Calling the gimplify_cilk_spawn on top of the gimplify_expr before the switch-statement could slow the compiler down 2. I need a CILK_SPAWN_STMT case in the switch statement in gimplify_expr (). 3. No test for catching the suspicious spawned function warning 4. Reasoning for expanding the 2 builtin functions in builtins.c instead of just inserting the appropriate expanded-code when I am inserting the function call. Did I miss anything else (or misunderstand anything you pointed out)? Here are my answers to those questions above and am attaching a fixed patch with the changelog entries: 1 2(partial): There are 3 places where we could have _Cilk_spawn: INIT_EXPR, CALL_EXPR and MODIFY_EXPR. INIT_EXPR and MODIFY_EXPRS are both gimplified using
RE: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and C++)
Hello, Has anyone had a chance to look at this. The C++ part is only a week old, but the C part has been in review for ~3 weeks. I would greatly appreciate if someone could review this and approve for trunk if it is Ok for trunk. Thanks, Balaji V. Iyer. -Original Message- From: Iyer, Balaji V Sent: Wednesday, September 11, 2013 2:18 PM To: r...@redhat.com; Jason Merrill (ja...@redhat.com); Jeff Law; 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++) 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? Thanks, Balaji V. Iyer. -Original Message- From: Iyer, Balaji V Sent: Friday, August 30, 2013 1:02 PM To: gcc-patches@gcc.gnu.org Subject: FW: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C The email seem to be bouncing gcc-patches. I have gzipped my patch. Thanks, Balaji V. Iyer. -Original Message- From: Iyer, Balaji V Sent: Friday, August 30, 2013 11:42 AM To: 'Aldy Hernandez' Cc: r...@redhat.com; Jeff Law; gcc-patches@gcc.gnu.org Subject: RE: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C Hi Aldy, Attached, please find a fixed patch and the changelog entries. -Original Message- From: Aldy Hernandez [mailto:al...@redhat.com] Sent: Wednesday, August 28, 2013 2:36 PM To: Iyer, Balaji V Cc: r...@redhat.com; Jeff Law; gcc-patches@gcc.gnu.org Subject: Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C On 08/27/13 16:27, Iyer, Balaji V wrote: Hello Aldy, I went through all the emails and here are the major issues that I could gather (other than lowering the keywords after gimplification, which I am skipping since it is more of an optimization for now). Ok, for now I am fine with delaying handling all this as a gimple tuple since most of your code lives in it's only little world :). But I will go on record saying that part of the reason that you have to handle CALL_EXPR, MODIFY_EXPR, INIT_EXPR and such is because you don't have easy gimplified code to examine. Anyways, agreed, you can do this later. 1. Calling the gimplify_cilk_spawn on top of the gimplify_expr before the switch-statement could slow the compiler down 2. I need a CILK_SPAWN_STMT case in the switch statement in gimplify_expr (). 3. No test for catching the suspicious spawned function warning 4. Reasoning for expanding the 2 builtin functions in builtins.c instead of just inserting the appropriate expanded-code when I am inserting the function call. Did I miss anything else (or misunderstand anything you pointed out)? Here are my answers to those questions above and am attaching a fixed patch with the changelog entries: 1 2(partial): There are 3 places where we could have _Cilk_spawn: INIT_EXPR, CALL_EXPR and MODIFY_EXPR. INIT_EXPR and MODIFY_EXPRS are both gimplified using gimplify_modify_expr. I have moved the cilk_detect_spawn into this function. We will go into the cilk_detect_spawn if cilk plus is enabled, and if there is a cilk_frame (meaning the function has a Cilk_spawn in it) thereby reducing the number of hits into this function significantly. Inside this function, it will go into the function that has a spawned function call and then unwrap the CILK_SPAWN_STMT wrapper and returns true. This shouldn't cause a huge compilation time hit. 2. To handle CALL_EXPR (e.g. _Cilk_spawn foo (x), where foo returns a void or the return value of it is ignored), I have added a CILK_SPAWN_STMT case. Again, I am calling the detect_cilk_spawn and we will only step into this function if Cilk Plus is enabled and if there is a cilk-frame (i.e saying the function has a cilk spawn in it). If there is an error (seen_error () == true), then it just falls through into CALL_EXPR and is handled like a normal call expr not spawned expression. 3. This warning rarely get
Re: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and C++)
On 09/17/2013 08:50 AM, Iyer, Balaji V wrote: Hello, Has anyone had a chance to look at this. The C++ part is only a week old, but the C part has been in review for ~3 weeks. I would greatly appreciate if someone could review this and approve for trunk if it is Ok for trunk. Obviously not yet. Everyone is pretty busy right now. jeff
