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

["Joseph S. Myers" <joseph at codesourcery dot com>]

On Thu, 8 Aug 2013, Iyer, Balaji V wrote:

> +enum add_variable_type  {

Two spaces before '{', should be one.

> +static HOST_WIDE_INT cilk_wrapper_count;

This is HOST_WIDE_INT but you use it later with sprintf with %ld; you need 
to use HOST_WIDE_INT_PRINT_DEC in such a case

> +  tree map = (tree)*map0;

There are several places like this where you are missing a space in a 
cast, which should be "(tree) *map0".

> +  /* Build the Cilk Stack Frame:
> +     struct __cilkrts_stack_frame {
> +       uint32_t flags;
> +       uint32_t size;
> +       struct __cilkrts_stack_frame *call_parent;
> +       __cilkrts_worker *worker;
> +       void *except_data;
> +       void *ctx[4];
> +       uint32_t mxcsr;
> +       uint16_t fpcsr;
> +       uint16_t reserved;
> +       __cilkrts_pedigree pedigree;
> +     };  */
> +
> +  tree frame = lang_hooks.types.make_type (RECORD_TYPE);
> +  tree frame_ptr = build_pointer_type (frame);
> +  tree worker_type = lang_hooks.types.make_type (RECORD_TYPE);
> +  tree worker_ptr = build_pointer_type (worker_type);
> +  tree s_type_node = build_int_cst (size_type_node, 4);
> +
> +  tree flags = add_field ("flags", unsigned_type_node, NULL_TREE);
> +  tree size = add_field ("size", unsigned_type_node, flags);

You refer to some fields as uint32_t above but then build them as unsigned 
int; you should be consistent.

I'm also suspicious of the "mxcsr" and "fpcsr" fields and associated enum 
values.  They don't really appear to be *used* for anything semantic in 
this patch - there's just boilerplate code dealing with creating them.  So 
I don't know what the point of them is at all - is there an associated 
runtime using them to be added by another patch in this series?  The 
problem is that they sound architecture-specific - they sound like they 
relate to registers on one particular architecture - meaning that they 
should actually be created by target hooks which might create different 
sets or sizes of such fields on different architectures (and they 
shouldn't appear at all in an enum in architecture-independent code, in 
that case).

> +  tree mxcsr = add_field ("mxcsr", uint32_type_node, context);
> +  tree fpcsr = add_field ("fpcsr", uint16_type_node, mxcsr);
> +  tree reserved = add_field ("reserved", uint16_type_node, fpcsr);

Note that uint32_type_node and uint16_type_node are internal types that 
may or may not correspond to the stdint.h C typedefs (one could be 
unsigned int and the other unsigned long, for example).  If this matters 
then you may need to work out how to use c_uint32_type_node etc. instead 
(but this code is outside the front end, so that may not be easy to do).  
(Cf. what I said in 
<http://gcc.gnu.org/ml/gcc-patches/2013-06/msg00248.html> about the 
remaining, presumably unmaintained, targets without stdint.h type 
information in GCC.)

> +       int32_t self;

> +  field = add_field ("self", unsigned_type_node, field);

Again, inconsistency of type.

> +Used to represent a spawning function in the Cilk Plus language extension.  
> +This tree has one field that holds the name of the spawning function.
> +_Cilk_spawn can be written in C in the following way:

@code{_Cilk_spawn} (and likewise _Cilk_sync), in several places.

> +Detailed description for usage and functionality of _Cilk_spawn can be found at
> +http://www.cilkplus.org

Use an actual link.

> 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
>  };
>  
> @@ -439,6 +440,8 @@ struct GTY(()) tree_base {
>    unsigned protected_flag : 1;
>    unsigned deprecated_flag : 1;
>    unsigned default_def_flag : 1;
> +  unsigned is_cilk_spawn : 1;
> +  unsigned is_cilk_spawn_detach_point : 1;

No, absolutely not.  This would expand all trees by a whole word.  Find a 
way to do whatever you need without increasing memory consumption like 
that.

> @@ -3496,7 +3508,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 : 3;
>  
>    unsigned static_ctor_flag : 1;
>    unsigned static_dtor_flag : 1;

Again, no.  See the comment "No bits left." at the bottom of this 
structure.  Expanding widely used datastructures is a bad idea, although 
this one isn't as bad to expand as tree_base.

-- 
Joseph S. Myers
joseph@codesourcery.com