RE: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C (and C++)

["Iyer, Balaji V" <balaji dot v dot iyer at intel dot com>]

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