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

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

Hi Aldy et al.,
	Attached, please find a fixed patch and the change-log entries below. Please see my answers to your questions below. The patch is tested on x86_64. So, is this Ok for trunk?

gcc/ChangeLog:
2013-08-13  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_valid_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_VALID_SPAWN): Likewise.
	(LANG_HOOKS_CILKPLUS_FRAME_CLEANUP): Likewise.
	(LANG_HOOKS_CILKPLUS_GIMPLIFY_SPAWN): Likewise.
	(LANG_HOOKS_CILKPLUS_GIMPLIFY_SYNC): Likewise.
	(LANG_HOOKS_CILKPLUS): Likewise.
	* builtin.def (DEF_CILK_BUILTIN_STUB): Likewise.
	* Makefile.in (C_COMMON_OBJS): Added c-family/cilk.o.
	(OBJS): Added cilk-common.o.
	* langhooks.c (lhd_install_body_with_frame_cleanup): New function.
	(lhd_cilk_valid_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): If a function call is a valid spawned function, then
	gimplify it using gimplify_cilk_spawn function call.  Also, added
	a CILK_SYNC_STMT case for gimplifying _Cilk_sync statement.
	* 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.
	* tree.h (CILK_SPAWN_FN): New #define.
	* 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-13  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_GIMPLIFY_SYNC): Likewise.
	(LANG_HOOKS_CILKPLUS_FRAME_CLEANUP): Likewise.
	(LANG_HOOKS_CILKPLUS_VALID_SPAWN): Likewise.
	* c-parser.c (c_parser_statement_after_labels): Added RID_CILK_SYNC
	case.
	(c_parser_postfix_expression): Added RID_CILK_SPAWN case.
	* c-tree.h (c_build_cilk_sync): New prototype.
	(c_build_cilk_spawn): Likewise.
	* 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.
	(c_build_cilk_spawn): New function.
	(c_build_cilk_sync): Likewise.

gcc/c-family/ChangeLog
2013-08-13  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_init_ts): Marked CILK_SPAWN_STMT and CILK_SYNC_STMT as typed.
	* 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.
	(gimplify_cilk_sync): Likewise.
	(c_cilk_install_body_w_frame_cleanup): Likewise.
	(cilk_valid_spawn): Likewise.
	(cilk_set_spawn_marker): Likewise.
	* cilk.c: New file.

gcc/lto/ChangeLog.cilk
2013-08-13  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.cilk
2013-08-13  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.
	* 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.


> -----Original Message-----
> From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-
> owner@gcc.gnu.org] On Behalf Of Aldy Hernandez
> Sent: Friday, August 09, 2013 6:40 AM
> 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
> 
> 
> 
> >>> --- gcc/expr.c
> >>> +++ gcc/expr.c
> >>> @@ -9569,6 +9569,21 @@ expand_expr_real_1 (tree exp, rtx target,
> >>> enum
> >> machine_mode tmode,
> >>>   	}
> >>>
> >>>         return expand_constructor (exp, target, modifier, false);
> >>> +    case INDIRECT_REF:
> >>> +      {
> >>> +	tree exp1 = TREE_OPERAND (exp, 0);
> >>> +	if (modifier != EXPAND_WRITE)
> >>> +	  {
> >>> +	    tree t = fold_read_from_constant_string (exp);
> >>> +	    if (t)
> >>> +	      return expand_expr (t, target, tmode, modifier);
> >>> +	  }
> >>> +	op0 = expand_expr (exp1, NULL_RTX, VOIDmode, EXPAND_SUM);
> >>> +	op0 = memory_address (mode, op0);
> >>> +	temp = gen_rtx_MEM (mode, op0);
> >>> +	set_mem_attributes (temp, exp, 0);
> >>> +	return temp;
> >>> +      }
> >>
> >> Ughhh, what's the rationale for this?  Are generic changes to
> >> expand_expr really needed?
> >
> > Yes, I am expanding some variables of type "ptr->value" and those are emitted
> as INDIRECT_REF.
> 
> The fact that you are getting an INDIRECT_REF this late in the game is suspect.
> 

I have removed handling of indirect_ref case in expr.c

> Are you building with ENABLE_CHECKING, because it seems this should have
> been caught.  See the places in tree-cfg.c with this:
> 
>      case INDIRECT_REF:
>        error ("INDIRECT_REF in gimple IL");
>        return t;
> >
> >>
> >>> +  /* During LTO, the is_cilk_function flag gets cleared.
> >>> +     If __cilkrts_pop_frame is called, then this definitely must be a
> >>> +     cilk function.  */
> >>> +  if (cfun)
> >>> +    cfun->is_cilk_function = 1;
> >>
> >> I don't know much about our LTO implementation, but perhaps you need
> >> to teach LTO to stream this bit in/out?  And of course, an
> >> accompanying LTO test to check for this problem you're encountering would
> be appropriate.
> >>
> >
> > I also have a limited knowledge of LTO. This seem to be the most
> straightforward way of doing it (atleast for me).
> 
> See how other bits in `struct function' are streamed in/out in LTO, for example in
> output_struct_function_base()
> 
>    bp_pack_value (&bp, fn->calls_alloca, 1);
>    bp_pack_value (&bp, fn->calls_setjmp, 1);
>    bp_pack_value (&bp, fn->va_list_fpr_size, 8);
>    bp_pack_value (&bp, fn->va_list_gpr_size, 8);
> 
> and the corresponding in input_struct_function_base():
> 
>    fn->calls_alloca = bp_unpack_value (&bp, 1);
>    fn->calls_setjmp = bp_unpack_value (&bp, 1);
>    fn->va_list_fpr_size = bp_unpack_value (&bp, 8);
>    fn->va_list_gpr_size = bp_unpack_value (&bp, 8);
> 
> Also, you will need a testcase to make sure later changes to the compiler do not
> break LTO wrt Cilk features you have added.
> 

I have added lto tests

> >
> >>> +       /* We need frame pointer for all Cilk Plus functions that uses
> >>> +	  Cilk Keywords.  */
> >>> +       || (flag_enable_cilkplus && cfun->is_cilk_function)
> >>
> >> "need a frame pointer"
> >>
> >> "that use"
> >>
> >> s/Keywords/keywords/
> >>
> >
> > It should be keywords, because you need frame-pointer for "_Cilk_spawn and
> _Cilk_sync" and "_Cilk_for"
> 
> I meant that you should lowercase the "K".
> 

This is fixed also

> >
> >>> +  /* This variable will tell whether we are on a spawn helper or
> >>> + not */  unsigned int is_cilk_helper_function : 1;
> >>
> >> Where is this used?
> >>
> >
> > Well, it is not used now but later on when I add Tools support it will be. I will
> remove it for now.
> 
> Yes, please.
> 

Yep, this is removed also.

> >>> --- gcc/ipa-inline-analysis.c
> >>> +++ gcc/ipa-inline-analysis.c
> >>> @@ -1433,6 +1433,9 @@ initialize_inline_failed (struct cgraph_edge *e)
> >>>       e->inline_failed = CIF_REDEFINED_EXTERN_INLINE;
> >>>     else if (e->call_stmt_cannot_inline_p)
> >>>       e->inline_failed = CIF_MISMATCHED_ARGUMENTS;
> >>> +  else if (flag_enable_cilkplus && cfun && cfun->calls_spawn)
> >>> +    /* We can't inline if the function is spawing a function.  */
> >>> +    e->inline_failed = CIF_BODY_NOT_AVAILABLE;
> >>
> >> Hmmm, if we don't have a cfun, perhaps we should be sticking this
> >> calls_spawn bit in the cgraph node.
> >>
> >> Richard?  Anyone?
> >>
> >
> > When I am first setting this, I don't think cgraph is available.
> 
> See Richard's comment with regards to struct function and its availability via the
> callee edge.  Also see his comment regarding the inappropriate error message.
> 

Yes, I changed it to FUNCTION_NOT_INLINABLE.

> >>> @@ -3496,7 +3510,7 @@ struct GTY(()) tree_function_decl {
> >>>        ???  The bitfield needs to be able to hold all target function
> >>>   	  codes as well.  */
> >>>     ENUM_BITFIELD(built_in_function) function_code : 11;
> >>> -  ENUM_BITFIELD(built_in_class) built_in_class : 2;
> >>> +  ENUM_BITFIELD(built_in_class) built_in_class  ;
> >>
> >> What's this for?
> >>
> >
> > Added a new enum field called BUILT_IN_CILK so we need 3 bits instead of 2,
> since there are 5 fields instead of 4.
> 
> Hmm, yeah.  I see you added another field here:
> 

Well, as requested by Joseph, I have removed this change.

> > diff --git a/gcc/tree.h b/gcc/tree.h
> > index 0058a4b..952362f 100644
> > --- a/gcc/tree.h
> > +++ b/gcc/tree.h
> > @@ -262,6 +262,7 @@ enum built_in_class
> >    NOT_BUILT_IN = 0,
> >    BUILT_IN_FRONTEND,
> >    BUILT_IN_MD,
> > +  BUILT_IN_CILK,
> >    BUILT_IN_NORMAL
> >  };
> 
> If you look at the comment above enum built_in_class, you will see that these
> classes specify which part of the compiler created the built-in (the frontend, the
> backend (MD), or a normal builtin).  I don't see how Cilk should be treated
> specially.  And even so, I don't see how you use this BUILT_IN_CILK class
> anywhere.
> 

This field is also removed as requested by Joseph.

> >>> +  if (DECL_BUILT_IN_CLASS (exp) == BUILT_IN_NORMAL)
> >>> +    return DECL_FUNCTION_CODE (exp) == BUILT_IN_MEMCPY;  return
> >>> + lang_hooks.cilkplus.spawnable_constructor (exp);  return false;
> >>
> >
> > That would be necessary for C++, but it returns false for C. So,
> > should I take out this hook for now? Would prefer to keep it in
> 
> You can keep the hook, but do put a comment specifying that it's a place holder
> for C++.  And also, please remove the second return.
> 
> >> Are these two hooks ever set to anything but hook_bool_tree_false?
> >> If so, why the need for them?
> >>
> >
> > Used in C++ but not in C.
> 
> Leave them.  Similar comment please.
> 

Added the comment in langhooks.h under its description.

> >>> +     struct __cilkrts_worker {
> >>> +       __cilkrts_stack_frame *volatile *volatile tail;
> >>> +       __cilkrts_stack_frame *volatile *volatile head;
> >>> +       __cilkrts_stack_frame *volatile *volatile exc;
> >>> +       __cilkrts_stack_frame *volatile *volatile protected_tail;
> >>> +       __cilkrts_stack_frame *volatile *ltq_limit;
> >>
> >> What's this "*volatile *volatile" stuff?
> >>
> >
> > That is how the field is described in Cilk Runtime source (please see:
> > http://gcc.gnu.org/svn/gcc/branches/cilkplus/libcilkrts/include/intern
> > al/abi.h)
> 
> Ok, I'm not a language expert, I presume this is specifying volatile on both
> indirections and is proper form.
> 
> >>> +/* Gimplifies the cilk_sync expression passed in *EXPR_P.  Returns
> >> GS_ALL_DONE
> >>> +   when finished.  */
> >>> +
> >>> +int
> >>> +gimplify_cilk_sync (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p
> >>> +		    ATTRIBUTE_UNUSED)
> >>> +{
> >>> +  tree sync_expr = build_cilk_sync ();
> >>> +  *expr_p = NULL_TREE;
> >>> +  gimplify_and_add (sync_expr, pre_p);
> >>> +  return GS_ALL_DONE;
> >>
> >> I'm not a big fan of functions that always return the same thing.
> >> The caller should set GS_ALL_DONE accordingly.
> >>
> >
> > The reason why I did this is that, there is a generic empty hook for this in
> langhooks.c that returns int. So, to reduce the number of code addition, I just
> made it return int. Also in future, if i want to add more things, having a return
> value I thought would be helpful.
> 
> Use void.  Set GS_ALL_DONE in the caller.  You can add the return value when
> you use it in the future.
> 

Thanks,

Balaji V. Iyer.


> Aldy

Attachment: diff.txt
Description: diff.txt