Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C

Attached, please find a fixed patch and ChangeLog entries.

My answers to your questions are given below.

Thanks,

Balaji V. Iyer. 

> -----Original Message-----
> From: Aldy Hernandez [mailto:aldyh@redhat.com]
> Sent: Monday, August 19, 2013 6:19 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
> 
> > @@ -960,6 +960,7 @@ SCEV_H = tree-scalar-evolution.h $(GGC_H)
> > tree-chrec.h $(PARAMS_H)  OMEGA_H = omega.h $(PARAMS_H)
> > TREE_DATA_REF_H = tree-data-ref.h $(OMEGA_H) graphds.h $(SCEV_H)
> > TREE_INLINE_H = tree-inline.h
> > +CILK_H = cilk.h
> >  REAL_H = real.h $(MACHMODE_H)
> >  IRA_INT_H = ira.h ira-int.h $(CFGLOOP_H) alloc-pool.h
> 
> Doesn't cilk.h depend on tree.h?  So shouldn't that be:
> 
> CILK_H = cilk.h $(TREE_H)
> 
> Which would mean that whoever includes cilk.h doesn't need to include tree.h.
> Although... what I would prefer is not to include tree.h from cilk.h at all, and
> have the caller/includer of cilk.h include tree.h first.
>

Ok. Fixed as you requested

> > +c-family/cilk.o : c-family/cilk.c $(TREE_H) $(SYSTEM_H) $(CONFIG_H)
> toplev.h \
> > +	$(TREE_H) coretypes.h tree-iterator.h $(TREE_INLINE_H) $(CGRAPH_H)
> \
> > +	$(DIAGNOSTIC_CORE_H) $(GIMPLE_H) $(CILK_H) $(C_COMMON_H)
> langhooks.h
> 
> TREE_H is duplicated.
> 

Fixed.

> Also, you are using DIAGNOSTIC_CORE_H whereas c-family/cilk.c is using
> 

Fixed.

> > +#include "diagnostic.h"
> 
> > +/* Cilk Keywords builtins.  */
> > +#include "cilk-builtins.def"
> 
> keywords should be lowercase
> 

Fixed.

> > diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h index
> > dc430c3..ab77fa4 100644
> > --- a/gcc/c-family/c-common.h
> > +++ b/gcc/c-family/c-common.h
> > @@ -148,6 +148,9 @@ enum rid
> >    /* C++11 */
> >    RID_CONSTEXPR, RID_DECLTYPE, RID_NOEXCEPT, RID_NULLPTR,
> > RID_STATIC_ASSERT,
> >
> > +  /* Cilk Plus Keywords.  */
> > +  RID_CILK_SPAWN, RID_CILK_SYNC,
> > +
> 
> Same here.

Fixed.

> 
> > +/* In cilk.c.  */
> > +extern tree insert_cilk_frame (tree); extern void cilk_init_builtins
> > +(void); extern int gimplify_cilk_spawn (tree *, gimple_seq *,
> > +gimple_seq *); extern int gimplify_cilk_sync (tree *, gimple_seq *,
> > +gimple_seq *); extern void c_cilk_install_body_w_frame_cleanup (tree,
> > +tree); extern bool cilk_valid_spawn (tree *); extern bool
> > +cilkplus_set_spawn_marker (location_t, tree);
> 
> You should be consistent with the prefix throughout the entire patch.
> For instance, now you have cilk_init_builtins() but cilkplus_set_spawn_marker().
> Do whatever you like, but be consistent.
> 
> Also, insert_cilk_frame-- the prefix should be well...as a prefix should be...at the
> beginning.  However, gimplify_cilk_spawn/sync is ok since that is in keeping with
> the rest of the gimplify.c style.
> 

I have renamed cilkplus_set_spawn_marker to cilk_set_spawn_marker.

Well, insert_cilk_frame seem to flow correctly than saying "cilk_insert_frame." Will it be OK if I have "cilk" somewhere. 

> > +  /* IF the function has _Cilk_spawn in front of a function call inside it
> > +     i.e. it is a spawning function, then add the appropriate Cilk plus
> > +     functions inside.  */
> 
> Lowercase the F in IF.
> 

Fixed.

> > +	  c_parser_skip_until_found (parser, CPP_SEMICOLON, "expected
> %<;%>");
> > +	  if (!flag_enable_cilkplus)
> > +	    error_at (loc, "-fcilkplus must be enabled to use _Cilk_sync");
> 
> The error message should use %<_Cilk_sync>
> 

Ok. I assume you wanted to make it %<_Cilk_sync%>....

> > +	case RID_CILK_SPAWN:
> > +	  c_parser_consume_token (parser);
> > +	  if (!flag_enable_cilkplus)
> > +	    {
> > +	      error_at (loc, "-fcilkplus must be enabled to use _Cilk_spawn");
> > +	      expr = c_parser_postfix_expression (parser);
> > +	      expr.value = error_mark_node;
> 
> Similarly with _Cilk_spawn.
> 

... did the same to _Cilk_spawn also.

> > +	  if (c_parser_peek_token (parser)->keyword == RID_CILK_SPAWN)
> > +	    {
> > +	      error_at (input_location, "consecutive _Cilk_spawn keywords "
> > +			"are not permitted");
> 
> Similarly here-- for that matter, check the rest of the patch for any keywords in
> error messages, they should have %<blah>.
> 
> Also, avoid input_location when possible.  Surely, you can infer the actual
> location from the parser.
> 

Yes, I have replaced them all, with the exception of gimplify.c. Please see my note below for why.

> > +  if (flag_enable_cilkplus && retval && TREE_CODE (retval) ==
> CILK_SPAWN_STMT)
> > +    {
> > +      error_at (loc, "use of _Cilk_spawn in return statement is not allowed");
> > +      return error_mark_node;
> 
> s/in return/in a return/.  Also, use %<_Cilk_spawn>.
> 
Fixed both.

> > +/* Returns a tree of type CILK_SYNC_STMT if Cilk Plus is enabled.  Otherwise
> > +   return error_mark_node.  */
> > +
> > +tree
> > +c_build_cilk_sync (void)
> > +{
> > +  tree sync = build0 (CILK_SYNC_STMT, void_type_node);
> > +  TREE_SIDE_EFFECTS (sync) = 1;
> > +  return sync;
> > +}
> 
> Code seems correct and comment wrong.  Adjust accordingly.
> 

Fixed.

> > diff --git a/gcc/cilk.h b/gcc/cilk.h
> > new file mode 100644
> > index 0000000..038316a
> > --- /dev/null
> > +++ b/gcc/cilk.h
> > @@ -0,0 +1,94 @@
> > +/* This file is part of the Intel(R) Cilk(TM) Plus support
> > +   This file contains Cilk Support files.
> > +   Copyright (C) 2013  Free Software Foundation, Inc.
> 
> Only one space after 2013.  Similarly in other files.
> 
Fixed.

> > +
> > +#ifndef GCC_CILK_H
> > +#define GCC_CILK_H
> > +
> > +#include "tree.h"
> 
> As mentioned earlier, cilk.h should not include tree.h, the caller should.
> 

Fixed.

> > +/* Frame status bits known to compiler.  */
> > +#define CILK_FRAME_STOLEN    0x01
> 
> Unused?
> 

Yup, so removed.

> > +this keyword with a set of functions that are stored in the Cilk Runtime.
> 
> Lowercase "Runtime".
> 

Fixed.

> > +@file{c-family/cilk.c}.  In the spawn-helper, the gimplification
> > +function @code{gimplify_call_expr}, inserts a function call
> @code{__cilkrts_detach}.
> > +This function is expanded by @code{builtin_expand_cilk_detach}
> > +located in @file{c-family/cilk.c}.
> > +
> > +@item @code{_Cilk_sync}:
> > +@code{_Cilk_sync} is parsed like a keyword.  During gimplification,
> > +the function @code{gimplify_cilk_sync} in @file{c-family/cilk.c},
> > +will replace this keyword with a set of functions that are stored in the Cilk
> Runtime.
> > +One of the internal functions inserted during gimplification,
> > +@code{__cilkrts_pop_frame} must be expanded by the compiler and is
> > +done by @code{builtin_expand_cilk_pop_frame} in @file{cilk-common.c}.
> 
> Here and elsewhere in the documentation... It's up to you, but I would prefer to
> remove reference to the actual file the functions are defined in, because
> functions could be rearranged later.
> 

I remember a while back someone was telling me that I should put that.

> > +	case CILK_SYNC_STMT:
> > +	  {
> > +	    if (flag_enable_cilkplus)
> > +	      {
> > +		if (!cfun->cilk_frame_decl)
> > +		  {
> > +		    error_at (input_location, "expected _Cilk_spawn before "
> > +			      "_Cilk_sync");
> > +		    ret = GS_ERROR;
> > +		  }
> > +		else
> > +		  ret = (enum gimplify_status)
> > +		    lang_hooks.cilkplus.gimplify_cilk_sync (expr_p, pre_p,
> > +							    post_p);
> > +		break;
> > +	      }
> > +	    else
> > +	      /* _Cilk_sync without Cilk Plus enabling should be caught by
> > +		 the parser.  */
> > +	      gcc_unreachable ();
> 
> The check for !flag_enable_cilkplus is not necessary, as well as the
> gcc_unreachable.  You shouldn't be checking for flag_enable_cilkplus in the
> gimplifier; an earlier pass (parser?) should've caught this.  See how
> TRANSACTION_EXPR is handled.
> 

OK. I fixed it.

> Also, I would use the location of the CILK_SYNC_STMT instead of
> input_location.
> 

In gimplify_expr function, the input_location is set to EXPR_LOCATION (*expr_p), which is the location of the CILK SYNC STMT.

> > diff --git a/gcc/ipa-inline-analysis.c b/gcc/ipa-inline-analysis.c
> > index 9a36292..7c63cc8 100644
> > --- a/gcc/ipa-inline-analysis.c
> > +++ b/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_cilk_spawn)
> > +    /* We can't inline if the function is spawing a function.  */
> > +    e->inline_failed = CIF_FUNCTION_NOT_INLINABLE;
> 
> Didn't rth mention that instead of looking through cfun (which may or may not
> be there), you could check the callee/caller edge?
> 

Yes, If I understood correctly, he said I was putting the wrong message in why inline failed. So I replaced it with CIF_FUNCTION_NOT_INLINABLE.

> > +void
> > +lhd_install_body_with_frame_cleanup (tree, tree) {
> > +  return;
> > +}
> 
> As mentioned before, avoid empty returns.
> 

Fixed.

> > +  /* Returns true if the call expr passed is a spawned function call.
> > + */
> 
> Here and elsewhere, you probably want s/call expr/CALL_EXPR/g
> 
Fixed.

> > diff --git a/gcc/tree-inline.h b/gcc/tree-inline.h index
> > b65dee9..32bedac 100644
> > --- a/gcc/tree-inline.h
> > +++ b/gcc/tree-inline.h
> > @@ -123,6 +123,10 @@ typedef struct copy_body_data
> >       the originals have been mapped to a value rather than to a
> >       variable.  */
> >    struct pointer_map_t *debug_map;
> > +
> > +  /* Cilk keywords currently need to replace some variables that
> > +     ordinary nested functions do not.  */  bool remap_var_for_cilk;
> >  } copy_body_data;
> >
> 
> Where is this field used?
> 

IN function cilk_outline in c-family/cilk.c

> > diff --git a/gcc/tree-pretty-print.c b/gcc/tree-pretty-print.c index
> > 7745f73..f449c68 100644
> > --- a/gcc/tree-pretty-print.c
> > +++ b/gcc/tree-pretty-print.c
> > @@ -2403,6 +2403,14 @@ dump_generic_node (pretty_printer *buffer, tree
> node, int spc, int flags,
> >        dump_block_node (buffer, node, spc, flags);
> >        break;
> >
> > +    case CILK_SPAWN_STMT:
> > +      pp_string (buffer, "_Cilk_spawn ");
> > +      dump_generic_node (buffer, TREE_OPERAND (node, 0), spc, flags, false);
> > +      break;
> > +
> > +    case CILK_SYNC_STMT:
> > +      pp_string (buffer, "_Cilk_sync;");
> > +      break;
> >      default:
> >        NIY;
> 
> 
> In keeping with the present code in this switch statement, empty line before the
> default.
> 

Fixed.

> > +/* Cilk spawn expression
> > +   Operand 0 is the CALL_EXPR.  */
> > +DEFTREECODE (CILK_SPAWN_STMT, "cilk_spawn_stmt", tcc_statement, 1)
> > +
> > +/* Cilk Sync Statement: Does not have any operands.  */ DEFTREECODE
> > +(CILK_SYNC_STMT, "cilk_sync_stmt", tcc_expression, 0)
> 
> Be consistent.  "Cilk spawn" but then "Cilk Sync"?  Also "expression" in one, but
> "statement" in the next?  Also, Statement should be lowercase, as well as Sync.
> 

Fixed.

> > +/* Cilk Keywords accessors.  */
> > +#define CILK_SPAWN_FN(NODE) TREE_OPERAND
> (CILK_SPAWN_STMT_CHECK
> > +(NODE), 0)
> > +
> 
> Lowercase "Keywords".
> 

Fixed.

> > +  if (!current_function_decl)
> > +    {
> 
> extra whitespace after {
> 

Fixed.

> > +  else if (TREE_CODE (fcall) != CALL_EXPR)
> > +    {
> 
> extra whitespace after {
> 

Fixed.

> > diff --git a/gcc/testsuite/c-c++-common/cilk-plus/CK/spawnee_inline.c
> > b/gcc/testsuite/c-c++-common/cilk-plus/CK/spawnee_inline.c
> > new file mode 100644
> > index 0000000..daf932e
> > --- /dev/null
> > +++ b/gcc/testsuite/c-c++-common/cilk-plus/CK/spawnee_inline.c
> > @@ -0,0 +1,81 @@
> > +/* { dg-do compile } */
> > +/* { dg-do run  { target { i?86-*-* x86_64-*-* arm*-*-* } } } */
> > +/* { dg-options "-fcilkplus -w" } */
> > +/* { dg-options "-lcilkrts" { target { i?86-*-* x86_64-*-* arm*-*-* }
> > +} } */
> > +
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#define DEFAULT_VALUE "30"
> > +
> > +int fib (char *n_char)
> > +{
> > +  int n;
> > +  char n_char_minus_one[20], n_char_minus_two[20];
> > +  if (n_char)
> > +    n = atoi (n_char);
> > +  else
> > +    n = atoi(DEFAULT_VALUE);
> > +
> > +  if (n < 2)
> > +    return n;
> > +  else
> > +    {
> 
> Extra whitespace after last {
> 
> > diff --git a/gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c
> > b/gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c
> > new file mode 100644
> > index 0000000..465d1da
> > --- /dev/null
> > +++ b/gcc/testsuite/c-c++-common/cilk-plus/CK/spawning_arg.c
> > @@ -0,0 +1,38 @@
> > +/* { dg-do compile } */
> > +/* { dg-do run  { target { i?86-*-* x86_64-*-* arm*-*-* } } } */
> > +/* { dg-options "-fcilkplus" } */
> > +/* { dg-options "-lcilkrts" { target { i?86-*-* x86_64-*-* arm*-*-* }
> > +} } */
> > +
> > +void f0(volatile int *steal_flag)
> > +{
> 
> same
> 
> > +/* This function does whatever is necessary to make the compiler emit
> > +a newly
> 
> Extra space at end
> 
> > +/* Return true if this is a tree which is allowed to contain a spawn as
> > +   operand 0.
> > +   A spawn call may be wrapped in a series of unary operations such
> > +   as conversions.  These conversions need not be "useless"
> > +   to be disregarded because they are retained in the spawned
> > +   statement.  They are bypassed only to look for a spawn
> > +   within.
> > +   A comparison to constant is simple enough to allow, and
> > +   is used to convert to bool.  */
> 
> Please add empty lines between paragraphs.
> 
> > +static bool
> > +cilk_spawnable_constructor (tree exp) {
> > +  if (TREE_CODE (exp) != ADDR_EXPR)
> > +    return false;
> 
> Is this function ever called with a non ADDR_EXPR?
> 

Fixed.

> > +/* Helper function for walk_tree.  If *TP is a CILK_SPAWN_STMT, then
> unwrap
> > +   this "wrapper" and  *WALK_SUBTREES is set to 0.  The function
> > +returns
> 
> two many spaces after "and"
> 
> Also, do not document what you are doing in the code by repeating it.
> For instance, instead of "*WALK_SUBTREES is set to 0", either omit this, or say
> what is being accomplished by setting WALK_SUBTREES to 0.  I would just omit
> it.
> 

Fixed.

> > +      /* Remove the CALL_EXPR from CILK_SPAWN_STMT wrapper and return
> > + true.  */
> 
> Again, don't document the obvious.  No need to say "and return true".
> That much is obvious from the code.
> > +  /* Happens with C++ TARGET_EXPR.  */  if (exp == NULL_TREE)
> > +    return false;
> 
> Extra space after false;
> 

Fixed.

> > +  warn = !cilk_spawnable_constructor (CALL_EXPR_FN (exp));
> > +
> > +  /* The function address of a call may not be computed via a spawn.
> > +     Look at the arglist only, and only the second argument which
> > +     is the RHS of any plausible assignment or copy.  The first
> > +     argument is the LHS.  A third argument could be a size for
> > +     memcpy.  This path supports op= in addition to =, only because
> > +     it is easy to do so. */
> > +  if (call_expr_nargs (exp) < 2)
> > +    return false;
> > +
> > +  exp = CALL_EXPR_ARG (exp, 0);
> > +
> > +  STRIP_USELESS_TYPE_CONVERSION (exp);
> > +
> > +  if (TREE_CODE (exp) == ADDR_EXPR)
> > +    exp = TREE_OPERAND (exp, 0);
> > +
> > +  if (TREE_CODE (exp) == TARGET_EXPR)
> > +    exp = TARGET_EXPR_INITIAL (exp);
> > +
> > +  if (!exp || !recognize_spawn (exp, exp0))
> > +    return false;
> > +
> > +  if (warn)
> > +    warning (0, "suspicious use of _Cilk_spawn");
> 
> This warning seems very ambiguous.  Do you think perhaps you could issue a
> more meaningful error in cilk_spawnable_constructor()?
> 

Fixed the warning

> > +/* This function will return a FNDECL using information from *WD.  */
> > +
> > +static tree
> > +build_cilk_helper_decl (struct wrapper_data *wd)
> 
> I think a more meaningful comment is "This function will build and return a
> FUNCTION_DECL using..."
> 

Fixed.

> > +    case LABEL_DECL:
> > +      error_at (EXPR_LOCATION (decl), "invalid use of label %q+D in spawn",
> > +		decl);
> > +      return error_mark_node;
> 
> Be consistent.  If you're using _Cilk_spawn then use that.  And you probably
> want %<_Cilk_spawn> or whatever.
> 

Fixed.

> > +  /* Copy FROM the function containing the spawn...  */  id.src_fn =
> > + outer_fn;
> > +
> > +  /* ...TO the wrapper.  */
> 
> Lower case FROM and TO.
> 
> 

In addition to this, I did the following from the other emails that I received:

1. Replaced all the build_* with create_* for the static functions used by the gimplify_cilk_sync and gimplify_cilk_spawn functions.
2. Removed semicolon from the pretty-print file for _Cilk_sync
3. Replaced the unwrap_cilk_sync_stmt with unwrap_cilk_spawn_stmt
4. Moved c_build_spawn_stmt and c_build_sync_stmt to cilk.c and dropped the c_ prefix.
5. Made cilk_call_setjmp a static function.
6. Make cilk_set_marker.. function inline.
7. I renamed cilk_valid_spawn to cilk_detect_spawn as I mentioned in the previous email. The recognize spawn is to check for spawn in C++ specific trees (e.g. AGGR_INIT_EXPR), and cilk_detect_spawn is to detect spawn in an expression. One is sort of the subset of the other. The only other thing I can think of is to have one function called "cilk_detect_spawn" and then redo this function for C++. But ~90% of the code will be replicated, which I think you (or someone in gcc mailing list) mentioned last time was not a good idea.
8. Did not mark (more like undid my previous marking of) CILK_SPAWN_STMT and CILK_SYNC_STMT as typed.

So, is this OK for trunk? Tested on x86_64 and works fine.

Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C

Commit Message

Patch