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

["Iyer, Balaji V" <balaji.v.iyer@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 12580 bytes --]

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 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 #2: ChangeLog.cilkplus --]
[-- Type: application/octet-stream, Size: 5708 bytes --]

gcc/ChangeLog:
2013-08-30  Balaji V. Iyer  <balaji.v.iyer@intel.com>

	* builtins.c (is_builtin_name): Added a check for __cilkrts_detach and
	__cilkrts_pop_frame.  If matched, then return true for built-in
	function name.
	(expand_builtin): Added BUILT_IN_CILK_DETACH and
	BUILT_IN_CILK_POP_FRAME case.
	* langhooks-def.h (lhd_install_body_with_frame_cleanup): New prototype.
	(lhs_cilk_detect_spawn): Likewise.
	(LANG_HOOKS_DECLS): Added LANG_HOOKS_CILKPLUS.
	(LANG_HOOKS_CILKPLUS_SPAWNABLE_CTOR): New #define.
	(LANG_HOOKS_CILKPLUS_RECOGNIZE_SPAWN): Likewise.
	(LANG_HOOKS_CILKPLUS_DETECT_SPAWN_AND_UNWRAP): Likewise.
	(LANG_HOOKS_CILKPLUS_FRAME_CLEANUP): Likewise.
	(LANG_HOOKS_CILKPLUS_GIMPLIFY_SPAWN): Likewise.
	(LANG_HOOKS_CILKPLUS): Likewise.
	* tree.h (CILK_SPAWN_FN): Likewise.
	* builtin.def (DEF_CILK_BUILTIN_STUB): Likewise.
	* Makefile.in (C_COMMON_OBJS): Added c-family/cilk.o.
	(OBJS): Added cilk-common.o.
	(CILK_H): Added a new define.
	(gimplify.o): Added CILK_H into dependency list.
	(builtins.o): Likewise.
	(ipa-inline.o): Likewise.
	(ipa-inline-analysis.o): Likewise.
	(BUILTINS_DEF): Added cilk-builtins.def.
	* langhooks.c (lhd_install_body_with_frame_cleanup): New function.
	(lhd_cilk_detect_spawn): Likewise.
	* langhooks.h (lang_hooks_for_cilkplus): New struct.
	(struct lang_hooks): Added new field called "cilkplus."
	* cilk-common.c: New file.
	* cilk.h: Likewise.
	* cilk-builtins.def: Likewise.
	* cppbuiltin.c (define_builtin_macros_for_compilation_flags): Added
	"__cilk" macro and set it to 200.
	* function.h (struct function::cilk_frame_decl): New field.
	(struct function::is_cilk_function): Likewise.
	(struct function::calls_cilk_spawn): Likewise.
	* gimplify.c (gimplify_call_expr): Added a check if the function call
	being gimplified is a spawn detach point.  If so, then add pop_frame
	and detach function calls.
	(gimplify_expr): Added a CILK_SPAWN_STMT and CILK_SYNC_STMT case
	for gimplifying _Cilk_spawn and _Cilk_sync statements.
	(gimplify_return_expr): Added a check for _Cilk_spawn usage in
	function.  If so, added a _Cilk_sync and gimplified it.
	(gimplify_modify_expr): Added a check for _Cilk_spawn in MODIFY and
	INIT_EXPRs.  If so, then call gimplify_cilk_spawn.
	* ipa-inline-analysis (initialize_inline_failed): Prevent inlining of
	spawner function.
	(can_inline_edge_p): Prevent inling of spawnee function.
	* ira.c (ira_setup_eliminable_regset): Force usage of frame pointer
	for functions that use Cilk keywords.
	* tree-inline.h (struct copy_body_data::remap_var_for_cilk): New field.
	* tree-pretty-print.c (dump_generic_node): Added CILK_SPAWN_STMT and
	CILK_SYNC_STMT cases.
	* tree.def (DEFTREECODE): Added CILK_SPAWN_STMT and CILK_SYNC_STMT
	trees.
	* generic.texi (CILK_SPAWN_STMT): Added documentation for _Cilk_spawn.
	(CILK_SYNC_STMT): Added documentation for _Cilk_sync.
	* passes.texi (Cilk Keywords): New section that describes the compiler
	code changes for handling Cilk Keywords.

gcc/c/ChangeLog:
2013-08-30  Balaji V. Iyer  <balaji.v.iyer@intel.com>

	* c-decl.c (finish_function): Added a call for insert_cilk_frame when
	a spawning function is found.
	* c-objc-common.h (LANG_HOOKS_CILKPLUS_GIMPLIFY_SPAWN): New #define.
	(LANG_HOOKS_CILKPLUS_FRAME_CLEANUP): Likewise.
	(LANG_HOOKS_CILKPLUS_DETECT_SPAWN_AND_UNWRAP): Likewise.
	* c-parser.c (c_parser_statement_after_labels): Added RID_CILK_SYNC
	case.
	(c_parser_postfix_expression): Added RID_CILK_SPAWN case.
	* c-typeck.c (build_compound_expr): Reject _Cilk_spawn in a comma
	expr.
	(c_finish_return): Added a check to reject _Cilk_spawn in return
	expression.
	(build_cilk_spawn): New function.
	(build_cilk_sync): Likewise.
	* Makefile.in (c-decl.o): Added cilk.h in dependency list.

gcc/c-family/ChangeLog
2013-08-30  Balaji V. Iyer  <balaji.v.iyer@intel.com>

	* c-common.c (c_common_reswords[]): Added _Cilk_spawn and _Cilk_sync
	fields.
	(c_define_builtins): Called cilk_init_builtins if Cilk Plus is
	enabled.
	* c-common.h (enum rid): Added RID_CILK_SPAWN and RID_CILK_SYNC.
	(insert_cilk_frame): New prototype.
	(cilk_init_builtins): Likewise.
	(gimplify_cilk_spawn): Likewise.
	(c_cilk_install_body_w_frame_cleanup): Likewise.
	(cilk_detect_spawn_and_unwrap): Likewise.
	(cilk_set_spawn_marker): Likewise.
	(build_cilk_sync): Likewise.
	(build_cilk_spawn): Likewise.
	* cilk.c: New file.

gcc/lto/ChangeLog
2013-08-30  Balaji V. Iyer  <balaji.v.iyer@intel.com>

	* Make-lang.in (lto/lto-lang.o): Added cilk.h in dependency list.
	* lto-lang.c (lto_init): Added a call to cilk_init_builtins if Cilk
	Plus is enabled.

gcc/testsuite/ChangeLog
2013-08-30  Balaji V. Iyer  <balaji.v.iyer@intel.com>

	* c-c++-common/cilk-plus/CK/compound_cilk_spawn.c: New test.
	* c-c++-common/cilk-plus/CK/concec_cilk_spawn.c: Likewise.
	* c-c++-common/cilk-plus/CK/fib.c: Likewise.
	* c-c++-common/cilk-plus/CK/no_args_error.c: Likewise.
	* c-c++-common/cilk-plus/CK/spawnee_inline.c: Likewise.
	* c-c++-common/cilk-plus/CK/spawner_inline.c: Likewise.
	* c-c++-common/cilk-plus/CK/spawning_arg.c: Likewise.
	* c-c++-common/cilk-plus/CK/steal_check.c: Likewise.
	* c-c++-common/cilk-plus/CK/test__cilk.c: Likewise.
	* c-c++-common/cilk-plus/CK/varargs_test.c: Likewise.
	* c-c++-common/cilk-plus/CK/sync_wo_spawn.c: Likewise.
	* c-c++-common/cilk-plus/CK/invalid_spawn.c: Likewise.
	* c-c++-common/cilk-plus/CK/spawn_in_return.c: Likewise.
	* c-c++-common/cilk-plus/CK/fib_init_expr_xy.c: Likewise.
	* c-c++-common/cilk-plus/CK/fib_no_sync.c: Likewise.
	* c-c++-common/cilk-plus/CK/fib_no_return.c: Likewise.
	* gcc.dg/cilk-plus/cilk-plus.exp: Added support to run Cilk Keywords
	test stored in c-c++-common.  Also, added the Cilk runtime's library
	to the ld_library_path.

[-- Attachment #3: diff.txt.gz --]
[-- Type: application/x-gzip, Size: 31543 bytes --]