From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19802 invoked by alias); 9 Aug 2013 16:52:22 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 19775 invoked by uid 89); 9 Aug 2013 16:52:21 -0000 X-Spam-SWARE-Status: No, score=-2.9 required=5.0 tests=AWL,BAYES_50,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,RDNS_NONE autolearn=no version=3.3.1 Received: from Unknown (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Fri, 09 Aug 2013 16:52:19 +0000 Received: from svr-orw-exc-10.mgc.mentorg.com ([147.34.98.58]) by relay1.mentorg.com with esmtp id 1V7pv8-0005Uu-W4 from joseph_myers@mentor.com ; Fri, 09 Aug 2013 09:52:11 -0700 Received: from SVR-IES-FEM-01.mgc.mentorg.com ([137.202.0.104]) by SVR-ORW-EXC-10.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Fri, 9 Aug 2013 09:52:10 -0700 Received: from digraph.polyomino.org.uk (137.202.0.76) by SVR-IES-FEM-01.mgc.mentorg.com (137.202.0.104) with Microsoft SMTP Server id 14.2.247.3; Fri, 9 Aug 2013 17:52:09 +0100 Received: from jsm28 (helo=localhost) by digraph.polyomino.org.uk with local-esmtp (Exim 4.76) (envelope-from ) id 1V7pv5-0007VX-Uu; Fri, 09 Aug 2013 16:52:08 +0000 Date: Fri, 09 Aug 2013 16:52:00 -0000 From: "Joseph S. Myers" To: "Iyer, Balaji V" CC: Aldy Hernandez , "rth@redhat.com" , Jeff Law , "gcc-patches@gcc.gnu.org" Subject: RE: [PATCH] Cilk Keywords (_Cilk_spawn and _Cilk_sync) for C In-Reply-To: Message-ID: References: <52012923.6030409@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" X-SW-Source: 2013-08/txt/msg00566.txt.bz2 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 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