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] |
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: rth@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:aldyh@redhat.com] > > > > Sent: Wednesday, August 28, 2013 2:36 PM > > > > To: Iyer, Balaji V > > > > Cc: rth@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 hit, and I > > > > > have seen it hit > > > > > > > > See my comments below on this. > > > > > > > > > only when you are spawning a constructor in C++. To my > > > > > knowledge, we have not had anyone report that they have hit this > > > > > warning. I just > > > > > > > > Ok, leave the warning, but do include a test case. > > > > > > > > > > Ok. Will do when I submit the C++ patch (since constructors are > > > really in C++) > > > > > > > > kept it in there just in case as a heads-up. 4. The reason why I > > > > > am handling pop_frame and detach functions in builtins.c is > > > > > because one of the things that LTO does is to remove the frame > > > > > pointer. All Cilk functions must use the frame pointer. When LTO > > > > > is invoked, it is hard to see what function is a cilk function > > > > > and what is not. The CFUN flag that says it is a cilk function > > > > > gets cleared out. But, the builtin functions are expanded during > > > > > LTO phase and I set the is_cilk_function flag when it is > > > > > expanding pop_frame and detach. The other option that I was > > > > > thinking was to have frame pointer on when Cilk Plus is enabled, > > > > > but this is a bit over-kill and can cause performance > > > issues. > > > > > > > > I think the reason you have to do all these gymnastics is > > > > bececause you are waiting so late to expand. You could expand > > > > into trees and not have to worry about frame pointers and such. > > > > See fold_builtin_* in builtins.c. *However*, if you think you can > > > > generate better code by delaying expansion all the way until RTL, > > > > then I'm fine with your current > > > approach. > > > > > > > > > > Well, one of the things the Cilk ABI requires is the usage of the frame > pointer. > > > LTO will remove frame pointer if the backend has > > > FRAME_POINTER_REQUIRED set to zero (which is true in x86 in general) > > regardless where I decompose things. > > > > > > > > > > > > > Also, I had added a couple more tests to catch a couple cases. > > > > > > > > > + /* Implicit _Cilk_sync must be inserted right before any return > statement > > > > > + if there is a _Cilk_spawn in the function (checked by seeing if > > > > > + cilk_frame_decl is not NULL_TREE). If the user has provided a > > > > > + _Cilk_sync, the optimizer should remove this duplicate one. > > > > > + */ if (flag_enable_cilkplus && cfun->cilk_frame_decl != > > > > > + NULL_TREE) > > > > > > > > Again, never document the obvious things your code is doing. For > > > > example, you can remove "(checked by seeing if > > > > > > Fixed. > > > > > > > > + cilk_frame_decl is not NULL_TREE)". It's obvious by looking at > > > > the code. > > > > > > > > > + /* If there are errors, there is no point in expanding the > > > > > + _Cilk_spawn. Just gimplify like a normal CALL_EXPR. */ > > > > > + && !seen_error ()) > > > > > > > > Similarly here. No need to document the obvious. > > > > > > > > > > Fixed. > > > . > > > > > + /* If there are errors, there is no point in expanding the > > > > > + _Cilk_spawn. Just gimplify like a normal MODIFY or INIT_EXPR. */ > > > > > + && !seen_error ()) > > > > > > > > And here. > > > > > > > > > > Fixed. > > > > > > > Alos, if the canonical way of checking if a function has a > > > > cilk_spawn in it is always "cfun->cilk_frame_decl != NULL_TREE", > > > > then you should abstract this into an inline function. The > > > > implementation will be trivial to change if we ever decide to keep > > > > that > > information elsewhere. > > > > Perhaps something like: > > > > > > > > static bool inline > > > > cilk_function_has_spawn (struct function *func) { > > > > return func->cilk_frame_decl != NULL_TREE; } > > > > > > > > > > OK. Fixed. > > > > > > > Do all these hooks you have in lang_hooks_for_cilkplus really have > > > > to be > > > hooks? > > > > That is, do you need different variants of them for C and for > > > > C++, or can you make do with one? Because if you only need one, > > > > C++there's > > > > no need for a hook. > > > > > > > Well, many of the the things are in language hook struct is because > > > C++ has exceptions and C doesn't. Thus, I need to add exception > > > related stuff into the > > > C++. > > > > > > The code to handle cilk_sync is same for C and C++. The reason why I > > > put it in the struct is for conformity (since spawn and sync go > > > together). I have taken it out of langhooks in the above patch. > > > > > > > > > > For example: > > > > > > > > > +int > > > > > +gimplify_cilk_sync (tree *expr_p, gimple_seq *pre_p, gimple_seq > *post_p > > > > > + ATTRIBUTE_UNUSED) > > > > > +{ > > > > > + tree sync_expr = expand_cilk_sync (); > > > > > + *expr_p = NULL_TREE; > > > > > + gimplify_and_add (sync_expr, pre_p); > > > > > + return GS_ALL_DONE; > > > > > +} > > > > > > > > The above is a hook. Will there be a different version of this for C++? > > > > If not, just call it directly. Similarly for > > > > gimplify_cilk_spawn(), and the rest of the hooks. > > > > > > > > > +/* Returns true if *EXP0 is a recognized form of spawn. > > > > > +Recognized forms > > > > are, > > > > > + after conversion to void, a call expression at outer level > > > > > + or an > > assignment > > > > > + at outer level with the right hand side being a spawned call. > > > > > + Note that `=' in C++ may turn into a CALL_EXPR rather than a > > > > MODIFY_EXPR. */ > > > > > + > > > > > +bool > > > > > +cilk_detect_spawn_in_expr (tree *exp0) { > > > > > > > > I don't like the name of this function or the corresponding hook > > > > (cilk_detect_spawn). You are actually detecting whether the > > > > expression contains a spawn *and* unwrapping it. So its use hides > > > > the fact that you are also modifying the expression in place. > > > > > > > Fixed as you suggested below. > > > > > > > > + if (flag_enable_cilkplus && cfun->cilk_frame_decl != NULL_TREE > > > > > + && lang_hooks.cilkplus.cilk_detect_spawn (expr_p) > > > > > + /* If there are errors, there is no point in expanding the > > > > > + _Cilk_spawn. Just gimplify like a normal CALL_EXPR. */ > > > > > + && !seen_error ()) > > > > > + return (enum gimplify_status) > > > > > + lang_hooks.cilkplus.gimplify_cilk_spawn (expr_p, pre_p, > > > > > + NULL); > > > > > > > > I would much rather have cilk_detect_spawn_and_unwrap() or something. > > > > Not pretty, but at least it doesn't hide what it's doing. That > > > > is, it's not just a check... it can transform the expression. > > > > > > > > Also, please document the fact that you are unwrapping and > > > > changing the expression in the comment to cilk_detect_spawn_in_expr. > > > > > > > > > > > > > Fixed. > > > > > > > > + case CILK_SPAWN_STMT: > > > > > + gcc_assert (flag_enable_cilkplus > > > > > + && cfun->cilk_frame_decl != NULL_TREE > > > > > + && lang_hooks.cilkplus.cilk_detect_spawn > (expr_p)); > > > > > + if (!seen_error ()) > > > > > + { > > > > > + ret = (enum gimplify_status) > > > > > + lang_hooks.cilkplus.gimplify_cilk_spawn (expr_p, > pre_p, > > > > > + post_p); > > > > > + break; > > > > > + } > > > > > > > > Remove the check for flag_enable_cilkplus. No one really builds > > > > that but cilk plus, and if for some reason it gets generated, it's > > > > pretty easy to debug/see why. See how we handled a slew of other > > > > front-end specific TREEs (like TRANSACTION_EXPR). No one ever > > > > checks for > > flag_*. > > > > > > > > > > OK. Fixed. > > > > > > > But BTW, your changes to gimplify.c are much better. It's a much > > > > cleaner approach. Thanks. > > > > > > > > > > Thanks! > > > > > > > > + case CILK_SYNC_STMT: > > > > > + { > > > > > + if (!cfun->cilk_frame_decl) > > > > > + { > > > > > + error_at (input_location, "expected %<_Cilk_spawn%> > before " > > > > > + "%<_Cilk_sync%>"); > > > > > + ret = GS_ERROR; > > > > > + } > > > > > > > > First, surely you have a location you can use, instead of the > > > > generic input_location (perhaps the location for the > > > > CILK_SYNC_STMT??). Also, Can you not check for this in > > > > c_finish_cilk_sync_stmt(), or the corresponding code-- that is, in > > > > the FE somewhere? And hopefully, in a place you can share with > > > > the > > > > C++ FE? If it is a real pain, I am willing to let this go, since > > > > C++ it > > > > happens only in the Cilk code path, though the general trend > > > > (especially with Andrew's proposed changes) is to do all type > > > > checking as > > close to the source as possible. > > > > > > If you look at the codes above (e.g. TRUTH_XOR_EXPR), they all use > > > input_location. Also, in the beginning of the function there is a line like this: > > > > > > if (save_expr != error_mark_node > > > && EXPR_HAS_LOCATION (*expr_p)) > > > input_location = EXPR_LOCATION (*expr_p); > > > > > > So, isn't input_location the same value as the location of the *expr_p? > > > > > > For the 2nd point, there can be a case where (with the help of > > > Gotos) _Cilk_sync can come before _Cilk_spawn. So, the only way to > > > do this check is to do it after the entire function is parsed. > > > > > > > > > > > > > > > > > Aldy
Attachment:
ChangeLog.cilkplus
Description: ChangeLog.cilkplus
Attachment:
patch_cilk_spawn_sync_c.txt.bz2
Description: patch_cilk_spawn_sync_c.txt.bz2
Attachment:
cp-ChangeLog.cilkplus
Description: cp-ChangeLog.cilkplus
Attachment:
patch_cilk_spawn_sync_c++.txt.bz2
Description: patch_cilk_spawn_sync_c++.txt.bz2
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |