From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10422 invoked by alias); 19 Oct 2012 21:55:06 -0000 Received: (qmail 10413 invoked by uid 22791); 19 Oct 2012 21:55:05 -0000 X-SWARE-Spam-Status: No, hits=-8.1 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mga02.intel.com (HELO mga02.intel.com) (134.134.136.20) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 19 Oct 2012 21:55:00 +0000 Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga101.jf.intel.com with ESMTP; 19 Oct 2012 14:55:00 -0700 X-ExtLoop1: 1 Received: from fmsmsx104.amr.corp.intel.com ([10.19.9.35]) by orsmga002.jf.intel.com with ESMTP; 19 Oct 2012 14:54:59 -0700 Received: from FMSMSX109.amr.corp.intel.com (10.19.9.28) by FMSMSX104.amr.corp.intel.com (10.19.9.35) with Microsoft SMTP Server (TLS) id 14.1.355.2; Fri, 19 Oct 2012 14:54:59 -0700 Received: from fmsmsx102.amr.corp.intel.com ([169.254.2.76]) by fmsmsx109.amr.corp.intel.com ([169.254.11.246]) with mapi id 14.01.0355.002; Fri, 19 Oct 2012 14:54:58 -0700 From: "Iyer, Balaji V" To: Joseph Myers CC: Richard Guenther , "gcc-patches@gcc.gnu.org" Subject: RE: [Ping]FW: [PATCH] Cilk Plus merging to trunk (2 of n) Date: Sat, 20 Oct 2012 00:14:00 -0000 Message-ID: References: In-Reply-To: Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-IsSubscribed: yes 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 X-SW-Source: 2012-10/txt/msg01843.txt.bz2 Hi Joseph, Thank you very much for your response. I will look into this and get back = to you soon! -Balaji V. Iyer. >-----Original Message----- >From: Joseph Myers [mailto:joseph@codesourcery.com] >Sent: Friday, October 19, 2012 5:38 PM >To: Iyer, Balaji V >Cc: Richard Guenther; gcc-patches@gcc.gnu.org >Subject: RE: [Ping]FW: [PATCH] Cilk Plus merging to trunk (2 of n) > >On Thu, 4 Oct 2012, Iyer, Balaji V wrote: > >> >>>>Here is a link to the latest spec. This should clear several of >> >>>>the questions you are seeking. >> >>>>(http://software.intel.com/sites/default/files/m/4/e/7/3/1/40297- >> >>>>Intel_Cilk_plus_lang_spec_2.htm#array) > >This specification is much improved, especially as regards specifying the = types of >section expressions. I'm not convinced that "the type of the array being >subscripted shall have a declared size" is properly defined in standard te= rms, but I >can guess reasonable semantics - that if the array-to-pointer decay were >considered not to occur in such a context, then the expressions for the ar= ray >being subscripted shall have array type, not pointer type, and the array t= ype shall >not be one with unspecified size (array[]), although it may be a VLA. For= example, >given "int a[10];", it would be valid to say a[:] or (a)[:] but not (+a)[:= ]. I don't, >however, see any testcases at all in this patch for that particular requir= ements - >not even for the completely clear-cut cases, such as giving an error for "= extern int >a[]; a[:];" or "int *a; a[:];". > >Say expr1 through expr9 are expressions with side effects, and you have: > >expr1[expr2:expr3:expr4] =3D expr5[expr6:expr7:expr8] + expr9; > >The spec says "However, in such a statement, a sub-expression with rank ze= ro is >evaluated only once." - that is, each of the nine expressions is evaluated= once. I >don't see any calls to save_expr to ensure these semantics, or any testcas= es that >verify that they are adhered to. > >(Are multidimensional section expressions valid when what you have is poin= ters >to pointers, e.g. "int ***p; p[0:10][0:10][0:10];"? I don't see anything = to rule >them out, so I assume they are valid, but don't see testcases for them eit= her.) > >Looking at the patch itself: > >In find_rank you have error ("Rank Mismatch!"); - this is not a properly f= ormatted >error message according to the GNU Coding standards (which typically would= not >have any uppercase). I'd also suggest that when you find a rank, you store >(through a location_t * pointer) the location of the first expression foun= d with >that rank, so if you then find a mismatching rank you can use error_at to = point to >that rank and then inform to point to the previous rank it didn't match. > >I'm not convinced that your logic, falling back to examining each operand = for a >generic expression, is correct to find the ranks of all kinds of expressio= ns. For >example, there are rules: > >* "The rank of a simple subscript expression (postfix-expression [ express= ion ]) is >the sum of the ranks of its operand expressions. The rank of the subscript >operand shall not be greater than one." - how do you ensure this rule? Wh= ere do >you test for errors if the subscript has too high a rank (both in the fron= t-end code, >and in the testsuite), and test (in the testsuite) for cases where the sub= script has >rank 1? > >* "The rank of a comma expression is the rank of its second operand." - I = don't >see anything special to handle that. Are there testcases for rank of comma >expressions? Apart from testing rank, you may need to test how they are >evaluated (that each part, with independent rank, gets fully evaluted in t= urn) - I >don't see anything obvious in the code to handle them appropriately. > >In general, I'd say you should have tests in the testsuite for each syntac= tic type of >expression supported by GCC, both standard and GNU extensions, testing how= it >interacts with section expressions - both valid cases, and cases that are = invalid >because of rank mismatches. As another example, you don't have tests of >conditional expressions. > >Where do you test (both in code, and testcases to verify errors) that "The= rank of >each expression in a section triplet shall be zero."? What about "The ran= k of the >postfix expression identifying the function to call shall be zero."? "A f= ull >expression shall have rank zero, unless it appears in an expression statem= ent or as >the controlling expression of an if statement."? (This means, I suppose, = that uses >such as initializers or sizes in array declarators must be rejected.) I'd= advise going >through each sentence in the relevant part of the spec that says something= is >invalid and making sure you diagnose it and have a test of this. > >Where in the patch you use int for the size of something (e.g. a list) on = the host, >please use size_t. > >In extract_array_notation_exprs you appear to be reallocating every time >something is added to a list (with XRESIZEVEC). It would probably be more >efficient to use the vec.h infrastructure for an automatically resizing ve= ctor on >which you push things. > >In c_parser_array_notation you appear to be converting indices to >integer_type_node in some cases, not converting at all in other cases. >But the spec says "The expressions in a triplet are converted to ptrdiff_t= .", so you >need to convert to target ptrdiff_t, not target int. >And there's a requirement that "Each of the expressions in a section tripl= et shall >have integer type.". So you need to check that, and give an error if it d= oesn't >have integer type, before converting - and of course add testcases for eac= h of >the possible positions for an expression having one that doesn't have inte= ger >type. > >In c-typeck.c you disable some errors and warnings for expressions contain= ing >array notations. I don't know where the later point is at which you check= for such >errors - but in any case, you need testcases for these diagnostics on thos= e cases >to show that they aren't being lost. > >In invoke.texi you have: > >+@opindex flag_enable_cilkplus > >But @opindex is for the user-visible options, not for internal variables. >That is, > >@opindex fcilkplus > >would be appropriate. > >In passes.texi you refer to "the Cilk runtime library (located in libcilkr= ts >directory)". But no such directory is added by this patch. >Only add references to it in documentation with the patch that adds the >directory. > >-- >Joseph S. Myers >joseph@codesourcery.com