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,
	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: rth@redhat.com; Jason Merrill (jason@redhat.com); Jeff Law; 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++)
> 
> 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,
> > > > > C++ since 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