RE: [PING]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>]

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