Can someone please review this patch for us? Thanks,
Balaji V. Iyer. > -----Original Message----- > From: Iyer, Balaji V > Sent: Monday, May 06, 2013 11:32 AM > To: Joseph S. Myers > Cc: 'Aldy Hernandez'; 'gcc-patches' > Subject: RE: [patch] cilkplus: Array notation for C patch > > Attached, please find a fixed patch. My responses to your comments are inline > below: > > Thanks, > > Balaji V. Iyer. > > Here are the ChangeLog entries: > > gcc/ChangeLog > +2013-05-06 Balaji V. Iyer <balaji.v.i...@intel.com> > + > + * doc/extend.texi (C Extensions): Added documentation about Cilk Plus > + array notation built-in reduction functions. > + * doc/passes.texi (Passes): Added documentation about changes done > + for Cilk Plus. > + * doc/invoke.texi (C Dialect Options): Added documentation about > + the -fcilkplus flag. > + * doc/generic.texi (Storage References): Added documentation for > + ARRAY_NOTATION_REF storage. > + * Makefile.in (C_COMMON_OBJS): Added c-family/array-notation- > common.o. > + (BUILTINS_DEF): Depend on cilkplus.def. > + * builtins.def: Include cilkplus.def. Define DEF_CILKPLUS_BUILTIN. > + * builtin-types.def: Define BT_FN_INT_PTR_PTR_PTR. > + * cilkplus.def: New file. > > gcc/c-family/ChangeLog > +2013-05-06 Balaji V. Iyer <balaji.v.i...@intel.com> > + > + * c-common.c (c_define_builtins): When cilkplus is enabled, the > + function array_notation_init_builtins is called. > + (c_common_init_ts): Added ARRAY_NOTATION_REF as typed. > + * c-common.def (ARRAY_NOTATION_REF): New tree. > + * c-common.h (build_array_notation_expr): New function declaration. > + (build_array_notation_ref): Likewise. > + (extract_sec_implicit_index_arg): New extern declaration. > + (is_sec_implicit_index_fn): Likewise. > + (ARRAY_NOTATION_CHECK): New define. > + (ARRAY_NOTATION_ARRAY): Likewise. > + (ARRAY_NOTATION_START): Likewise. > + (ARRAY_NOTATION_LENGTH): Likewise. > + (ARRAY_NOTATION_STRIDE): Likewise. > + (ARRAY_NOTATION_TYPE): Likewise. > + * c-pretty-print.c (pp_c_postifix_expression): Added a new case for > + ARRAY_NOTATION_REF. > + (pp_c_expression): Likewise. > + * c.opt (flag_enable_cilkplus): New flag. > + * array-notation-common.c: New file. > > gcc/c/ChangeLog > +2013-05-06 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 and 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. > + (build_binary_op): Added a check for array notation expr inside > + op1 and op0. If present, we call another function to find correct > + type. > + * 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): Similarly, but for a while. > + (c_parser_do_statement): Similarly, but for a do-while. > + (c_parser_for_statement): Similarly, 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. > + * c-tree.h (is_cilkplus_reduce_builtin): Protoize. > > > > -----Original Message----- > > From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches- > > ow...@gcc.gnu.org] On Behalf Of Joseph S. Myers > > Sent: Monday, April 29, 2013 6:55 PM > > To: Iyer, Balaji V > > Cc: 'Aldy Hernandez'; 'gcc-patches' > > Subject: RE: [patch] cilkplus: Array notation for C patch > > > > Here's a review of the changes to the compiler proper in this patch. > > I don't think much more will come up from reviews of the compiler > > changes - but I still need to review the testsuite changes against the > > language specification to make sure that everything is properly > > covered in the testsuite (which might in turn show up further things > > needing to > be addressed in the compiler). > > > > > + error_at (location, "__sec_implicit_index parameter must be a " > > > + "integer constant expression"); > > > > "an", not "a". > > FIXED! > > > > > > diff --git a/gcc/c/ChangeLog.cilkplus b/gcc/c/ChangeLog.cilkplus > > > > I believe the actual trunk commit, when this is ready to go in, should > > simply add the ChangeLog entries for the committed changes to the top > > of the existing ChangeLog files, rather than creating such a new ChangeLog > file. > > > > > diff --git a/gcc/c/c-array-notation.c b/gcc/c/c-array-notation.c > > > > > +#include "gcc.h" > > > > That header is for the compiler driver. Including it in anything > > built into cc1 is suspicious. > > Removed. This was an unnecessary #include. > > > > > > +/* Given an FNDECL or an ADDR_EXPR, return the corresponding > > > > I think you mean something like "Given FNDECL, a FUNCTION_DECL or an > > ADDR_EXPR", rather than "Given an FNDECL or an ADDR_EXPR". > > Yep. This is fixed. > > > > > > +/* Set *RANK of expression ARRAY, ignoring array notation specific > > > built-in > > > + functions if IGNORE_BUILTIN_FN is true. The ORIG_EXPR is > > > +printed out if > > an > > > + error occured in the rank calculation. The functions returns false > > > if it > > > + encounters an error in rank calculation. > > > + > > > + For example, an array notation of A[:][:] or B[0:10][0:5:2] or > > > C[5][:][1:0] > > > + all have a rank of 2. */ > > > > This still doesn't seem to say anything about the semantics of the > > value *RANK on entry to the function. (I think it's something like > > *RANK being either 0, or the rank of another subexpression that must > > have the same rank as this one, but you need to say that.) > > I have reworded this, please let me know if it is OK. > > > > > > +/* Extracts all array notations in NODE and stores them in ARRAY_LIST. > > > If > > > + IGNORE_BUILTIN_FN is set, then array notations inside array notation > > > + specific built-in functions are ignored. The NODE can be anything > > > from a > > > + full function to a single variable. */ > > > > "can be anything"? That seems rather ad hoc. I'd think there should > > be defined classes of trees - probably expressions and things that can > > appear in them, but not tcc_exceptional or tcc_type - that can appear > > here, and that you should check (in an assertion) for EXPR_P or one of the > other cases allowed. > > I have made this more specific > > > > > In particular, you allow TREE_LIST in this function. How can > > TREE_LISTs get here and can they readily be avoided? It's generally a > > bad idea (and rare) to have places where something with the static > > type "tree" can be either a TREE_LIST or some other kind of tree. I > > note that in the function replace_array_notations, which is presumably > > intended to match this one, you > > *don't* handle TREE_LIST. > > I removed TREE_LIST checks. > > > > > These functions recurse down into operands of trees. But what about > > into types? If a type contains an expression that needs to be > > evaluated as part of evaluating VLA sizes, that gets stored specially > > by grokdeclarator, and in the end that expression get put in a > > statement somewhere to ensure that it does get evaluated. But that's > > for expressions with side effects involved in types. Array notation > > expressions may not necessarily have side effects. And as I > > understand it, even if an expression is extracted OK by > > extract_array_notation_exprs because it appears somewhere that > > function looks at, replace_array_notations will need to substitute it > everywhere - substituting a copy appearing directly in a statement / > expression, > while missing a copy embedded in a type, won't suffice. > > So maybe you need to recurse down into types in some way? (Then I'm > > not entirely sure when it's safe to modify an existing type and when > > you'd need to build up a new, similar type with the expression > > modified appropriately.) > > > > Maybe an example would help. I see nothing in the Cilk Plus > > specification to rule out expressions of the form > > > > a[:] = ((int (*)[b[:]][c[:]]) d[:])[1][2]; > > > > meaning that each element of the array d should be cast to a > > pointer-to-VLA type, with the dimensions of the VLA coming from > > corresponding elements of arrays b and c, and then element[1][2] of > > that VLA extracted. But the rules for determining rank don't really > > seem to consider subexpressions that appear within types, so maybe > > adjustments are needed there as well. (Of course such type names can > > appear within expressions in sizeof, or compound literals, or several > > other cases in the syntax, not just in casts.) > > > > It's possible that the above case does work despite types not being > > adjusted, because the logic to multiply by array sizes when doing > > pointer addition / array dereference may already have taken effect > > while the expressions were constructed. But leaving types unadjusted > > still seems rather risky, and would seem likely to cause problems with > > debug info (consider the case where a variable is actually being > > declared with the type involving array notation, in a GNU C statement > expression, for example) if nothing else. > > > > (I suppose changing the specification to disallow array notation > > within types would be one way to avoid a lot of such issues.) > > Yes, you can't have array notation in types. We will fix this in the next > revision of > the manual. > > > > > > +/* Top-level function to replace ARRAY_NOTATION_REF in a > > > +conditional > > statement > > > + in STMT. */ > > > + > > > +tree > > > +fix_conditional_array_notations (tree stmt) > > > > This comment doesn't seem to say anything abou the return value semantics. > > FIXED! > > > > > > +/* Returns true of EXPR (and its subtrees) contain > > > +ARRAY_NOTATION_EXPR node. */ > > > > "true if", not "true of". > > FIXED! > > > > > > +/* Walks through tree node T and find all the call-statments that > > > +do not return > > > > Statements, not statments. > > FIXED! > > > > > > diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c index > > > 2ae4622..f051ab5 100644 > > > --- a/gcc/c/c-parser.c > > > +++ b/gcc/c/c-parser.c > > > @@ -55,6 +55,7 @@ along with GCC; see the file COPYING3. If not see > > > #include "cgraph.h" > > > #include "plugin.h" > > > > > > + > > > > > > > /* Initialization routine for this file. */ > > > > > > > Spurious diff hunk just adding whitespace. > > FIXED! > > > > > > + if (flag_enable_cilkplus && contains_array_notation_expr (cond)) > > > + { > > > + error_at (start_locus, "array notation expression cannot be used > > > in a " > > > + "loop%'s condition"); > > > + return; > > > + } > > > + if (flag_enable_cilkplus && contains_array_notation_expr (incr) && 0) > > > + { > > > + error_at (start_locus, "array notation expression cannot be used > > > in a " > > > + "loop's increment expression"); > > > + return; > > > + } > > > > Use %' in the second error here, as you did in the first. > > > > Removed the 2nd if statement, thus this issue is solved. > > > > diff --git a/gcc/tree-pretty-print.c b/gcc/tree-pretty-print.c > > > > > + case ARRAY_NOTATION_REF: > > > > Since this is a C-family tree node, I'd think the handling in > > c-pretty-print.c should suffice, without needing anything in > > tree-pretty-print.c > as well. > > Removed! > > > > > -- > > Joseph S. Myers > > jos...@codesourcery.com
