Hi Joseph,
The fixed patch and the changelogs are attached in this
email(http://gcc.gnu.org/ml/gcc-patches/2013-08/msg00758.html). I have
addressed your concerns below:
Thanks,
Balaji V. Iyer.
> -----Original Message-----
> From: Joseph Myers [mailto:jos...@codesourcery.com]
> Sent: Friday, August 09, 2013 12:52 PM
> To: Iyer, Balaji V
> Cc: Aldy Hernandez; r...@redhat.com; Jeff Law; gcc-patches@gcc.gnu.org
> Subject: RE: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C
>
> 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".
Fixed!
>
> > + /* 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.
>
Fixed.
> 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).
>
They are removed.
> > + 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.
>
Fixed
> > +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.
Fixed.
>
> > +Detailed description for usage and functionality of _Cilk_spawn can
> > +be found at http://www.cilkplus.org
>
> Use an actual link.
>
We have recently launched the website, so the actual link might change. I will
change it to the following:
Documentation about Cilk Plus and language specification is provided under the
"Learn" section in @w{@uref{http://www.cilkplus.org/}}.
> > 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.
>
OK. Removed.
> > @@ -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.
>
Ok. Removed this one too.
> --
> Joseph S. Myers
> jos...@codesourcery.com
