On Tue, Dec 11, 2012 at 6:50 PM, Iyer, Balaji V <balaji.v.i...@intel.com> wrote: > Hello, > Did you get a chance to look at this patch? I submitted this ~1 month > ago, so thought I would inquire its status.
Let me chime in from a release manager POV - I fear this needs to wait for next stage1, we're way into stage3 to get such major features in. Thanks, Richard. > Thanks, > > Balaji V. Iyer. > >> -----Original Message----- >> From: Iyer, Balaji V >> Sent: Monday, November 05, 2012 4:43 PM >> To: Joseph Myers >> Cc: gcc-patches@gcc.gnu.org >> Subject: RE: [Ping]FW: [PATCH] Cilk Plus merging to trunk (2 of n) >> >> Hello Joseph, >> Here is the fixed patch with all your changes and the ChangeLog entries >> below. >> >> gcc/ChangeLog >> 2012-11-05 Balaji V. Iyer <balaji.v.i...@intel.com> >> >> * Makefile.in (C_COMMON_OBJS): Added c-family/array-notation- >> common.o. >> * doc/passes.texi (Cilk Plus Transformation): Documented array >> notation and overall transformations for Cilk Plus. >> * doc/invoke.texi (C Dialect Options): Documented -fcilkplus flag. >> * doc/generic.texi (Storage References): Documented >> ARRAY_NOTATION_REF >> tree addition. >> >> gcc/c-family/ChangeLog >> 2012-11-05 Balaji V. Iyer <balaji.v.i...@intel.com> >> >> * c-common.h (build_array_notation_expr): New function declaration. >> (ARRAY_NOTATION_ARRAY): Added new #define. >> (ARRAY_NOTATION_CHECK): Likewise. >> (ARRAY_NOTATION_START): Likewise. >> (ARRAY_NOTATION_LENGTH): Likewise. >> (ARRAY_NOTATION_STRIDE): Likewise. >> (ARRAY_NOTATION_TYPE): Likewise. >> (enum array_notation_reduce_type): Added new enumerator. >> * c-common.def: Added new tree ARRAY_NOTATION_REF. >> * c-common.c (c_define_builtins): Added a call to initialize array >> notation builtin functions. >> (c_common_init_ts): Set ARRAY_NOTATION_REF as typed. >> * c-pretty-print.c (pp_c_postfix_expression): Added >> ARRAY_NOTATION_REF >> case. >> * c.opt (-fcilkplus): Define new command line switch. >> * array-notation-common.c: New file. >> >> gcc/c/ChangeLog >> 2012-11-05 Balaji V. Iyer <balaji.v.i...@intel.com> >> >> * c-typeck.c (build_array_ref): Added a check to see if array's index >> is greater than one. If true, then emit an error. >> (build_function_call_vec): Exclude error reporting & checking for >> builtin array-notation functions. >> (convert_arguments): Likewise. >> (c_finish_return): Added a check for array notations as a return >> expression. If true, then emit an error. >> (c_finish_loop): Added a check for array notations in a loop >> condition. >> If true then emit an error. >> (lvalue_p): Added a ARRAY_NOTATION_REF case. >> * Make-lang.in (C_AND_OBJC_OBJS): Added c-array-notation.o. >> * c-parser.c (c_parser_compound_statement): Check if array notation >> code >> is used in tree, if so, then transform them into appropriate C code. >> (c_parser_expr_no_commas): Check if array notation is used in LHS or >> RHS, if so, then build array notation expression instead of regular >> modify. >> (c_parser_postfix_expression_after_primary): Added a check for >> colon(s) >> after square braces, if so then handle it like an array notation. >> Also, >> break up array notations in unary op if found. >> (c_parser_direct_declarator_inner): Added a check for array notation. >> (c_parser_compound_statement): Added a check for array notation in a >> stmt. If one is present, then expand array notation expr. >> (c_parser_if_statement): Likewise. >> (c_parser_switch_statement): Added a check for array notations in a >> switch statement's condition. If true, then output an error. >> (c_parser_while_statement): Same as switch statement, but for a >> while. >> (c_parser_do_statement): Same as switch statement, but for a >> do-while. >> (c_parser_for_statement): Same as switch statement, but for a >> for-loop. >> (c_parser_unary_expression): Check if array notation is used in a >> pre-increment or pre-decrement expression. If true, then expand >> them. >> (c_parser_array_notation): New function. >> * c-array-notation.c: New file. >> >> gcc/testsuite/ChangeLog >> 2012-11-05 Balaji V. Iyer <balaji.v.i...@intel.com> >> >> * gcc.dg/cilk-plus/array_notation/execute/execute.exp: New script. >> * gcc.dg/cilk-plus/array_notation/compile/compile.exp: Likewise. >> * gcc.dg/cilk-plus/array_notation/errors/errors.exp: Likewise. >> * gcc.dg/cilk-plus/array_notation/execute/sec_implicit_ex.c: New >> test. >> * gcc.dg/cilk-plus/array_notation/execute/comma_exp.c: Likewise. >> * gcc.dg/cilk-plus/array_notation/execute/conditional.c: Likewise. >> * gcc.dg/cilk-plus/array_notation/execute/exec-once.c: Likewise. >> * gcc.dg/cilk-plus/array_notation/execute/if_test.c: Likewise. >> * gcc.dg/cilk-plus/array_notation/execute/n-ptr_test.c: Likewise. >> * gcc.dg/cilk-plus/array_notation/execute/gather_scatter.c: Likewise. >> * gcc.dg/cilk-plus/array_notation/execute/builtin_func_double2.c: >> Likewise. >> * gcc.dg/cilk-plus/array_notation/execute/builtin_func_double.c: >> Likewise. >> * gcc.dg/cilk-plus/array_notation/execute/builtin_fn_custom.c: >> Likewise. >> * gcc.dg/cilk-plus/array_notation/execute/builtin_fn_mutating.c: >> Likewise. >> * gcc.dg/cilk-plus/array_notation/execute/array_test_ND.c: Likewise. >> * gcc.dg/cilk-plus/array_notation/execute/array_test2.c: Likewise. >> * gcc.dg/cilk-plus/array_notation/execute/array_test1.c: Likewise. >> * gcc.dg/cilk-plus/array_notation/compile/sec_implicit_ex.c: >> Likewise. >> * gcc.dg/cilk-plus/array_notation/compile/gather_scatter.c: Likewise. >> * gcc.dg/cilk-plus/array_notation/compile/builtin_func_double2.c: >> Likewise. >> * gcc.dg/cilk-plus/array_notation/compile/array_test_ND.c: Likewise. >> * gcc.dg/cilk-plus/array_notation/compile/if_test.c: Likewise. >> * gcc.dg/cilk-plus/array_notation/compile/builtin_func_double.c: >> Likewise. >> * gcc.dg/cilk-plus/array_notation/compile/array_test1.c: Likewise >> * gcc.dg/cilk-plus/array_notation/compile/array_test2.c: Likewise. >> * gcc.dg/cilk-plus/array_notation/errors/sec_implicit.c: Likewise. >> * gcc.dg/cilk-plus/array_notation/errors/sec_implicit2.c: Likewise. >> * gcc.dg/cilk-plus/array_notation/errors/rank_mismatch.c: Likewise. >> * gcc.dg/cilk-plus/array_notation/errors/parse_error.c: Likewise. >> * gcc.dg/cilk-plus/array_notation/errors/parse_error2.c: Likewise. >> * gcc.dg/cilk-plus/array_notation/errors/parse_error3.c: Likewise. >> * gcc.dg/cilk-plus/array_notation/errors/parse_error4.c: Likewise. >> * gcc.dg/cilk-plus/array_notation/errors/sec_reduce_max_min_ind.c: >> Likewise. >> * gcc.dg/cilk-plus/array_notation/errors/decl-ptr-colon.c: Likewise. >> * gcc.dg/cilk-plus/array_notation/errors/fn_ptr.c: Likewise. >> * gcc.dg/cilk-plus/array_notation/errors/fn_triplet_values.c: >> Likewise. >> * gcc.dg/cilk-plus/array_notation/errors/gather-scatter.c: Likewise. >> * gcc.dg/cilk-plus/array_notation/errors/misc.c: Likewise. >> * gcc.dg/cilk-plus/array_notation/errors/vla.c: Likewise. >> >> Thanks, >> >> Balaji V. Iyer. >> >> >> > -----Original Message----- >> > From: Joseph Myers [mailto:jos...@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 terms, 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 array being subscripted shall >> > have array type, not pointer type, and the array type 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 requirements - 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] = expr5[expr6:expr7:expr8] + expr9; >> > >> > The spec says "However, in such a statement, a sub-expression with >> > rank zero 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 testcases that verify that they are adhered >> > to. >> > >> > (Are multidimensional section expressions valid when what you have is >> > pointers 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 either.) >> > >> > Looking at the patch itself: >> > >> > In find_rank you have error ("Rank Mismatch!"); - this is not a >> > properly formatted 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 found 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 expressions. For example, there are rules: >> > >> > * "The rank of a simple subscript expression (postfix-expression [ >> > expression ]) 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? Where do you test for errors if the subscript >> > has too high a rank (both in the front-end code, and in the >> > testsuite), and test (in the testsuite) for cases where the subscript 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 turn) - 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 >> > syntactic 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 rank of the postfix expression identifying the >> > function to call shall be zero."? "A full expression shall have rank >> > zero, unless it appears in an expression statement 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 vector 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 >> > triplet shall have integer type.". So you need to check that, and >> > give an error if it doesn't have integer type, before converting - and >> > of course add testcases for each of the possible positions for an >> > expression having one that doesn't have integer type. >> > >> > In c-typeck.c you disable some errors and warnings for expressions >> > containing 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 those 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 >> > libcilkrts 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 >> > jos...@codesourcery.com