This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
Hi Aldy et al., I am sorry but I forgot to cut and paste the ChangeLog entries in my previous reply. I am also attaching the patch again, so that they can all be in the same message. Here you go: gcc/cp/ChangeLog 2013-06-12 Balaji V. Iyer <balaji.v.iyer@intel.com> * call.c (convert_like_real): Added a check if array notation is present in expression. If so, then no conversion of arguments is necessary. (build_over_call): Likewise. * typeck.c (cp_build_function_call_vec): Likewise. (convert_for_assignment): Likewise. (cp_build_array_ref): Reject array notations with a rank greater than 1 as an array's index. (cp_build_binary_op): If array notations are preent in op, then call find_correct_array_notation_type. (cp_build_addr_expr_1): Handle ARRAY_NOTATION_REF similar to ARRAY_REF. * cp-array-notation.c: New file. * cp-objcp-common.c (cp_common_init_ts): Marked ARRAY_NOTATION_REF tree as typed. * cp-tree.h (fix_array_notation_exprs): New prototype. * semantics.c (finish_return_stmt): Reject array notations as return value. (cxx_eval_constant_expression): Added ARRAY_NOTATION_REF case. (potential_constant_expression_1): Likewise. * tree.c (lvalue_kind): Likewise. * error.c (dump_decl): Likewise. (dump_expr): Likewise. * pt.c (ARRAY_NOTATION_REF): Likewise. (type_unification_real): Do not unify any arguments if array notations are found in arg. (instantiate_decl): Added a check for array notaitons inside the function body. If so, then expand them. * parser.c (cp_parser_array_notation): New function. (cp_parser_postfix_open_square_expression): Added a check for colons inside square braces. If found, then handle the array access as an array notation access. Also, disable auto-correction from a single colon to scope when Cilk Plus is enabled. (cp_parser_compound_statement): Added a check for array notations inside the statement. If found, then expand them. (cp_parser_ctor_initializer_opt_and_function_body): Likewise. (cp_parser_function_definition_after_declarator): Likewise. (cp_parser_selection_statement): Searched for array notations inside condition. If so, then emit an error. (cp_parser_iteration_statement): Likewise. (cp_parser_direct_declarator): Reject array notations inside a variable or array declaration. * Make-lang.in (CXX_AND_OBJCXX_OBJS): Added cp/cp-array-notation.o. gcc/testsuite/ChangeLog 2013-06-12 Balaji V. Iyer <balaji.v.iyer@intel.com> * c-c++-common/cilk-plus/AN/array_test1.c: Make this an execution test. Also changed the returns from error as distinct values so that debugging can get easier. * c-c++-common/cilk-plus/AN/if_test_errors.c (main): Made certain errors specific to C, if necessary. Also added new error hooks for C++. * c-c++-common/cilk-plus/AN/misc.c (main): Likewise. * c-c++-common/cilk-plus/AN/parser_errors.c (main): Likewise. * c-c++-common/cilk-plus/AN/parser_errors2.c (main): Likewise. * c-c++-common/cilk-plus/AN/parser_errors3.c (main): Likewise. * c-c++-common/cilk-plus/AN/pr57541.c (main): Likewise. * c-c++-common/cilk-plus/AN/parser_errors4.c (main): In addition to the same changes as parser_errors3.c, spaces were added between colons to not confuse C++ compiler with 2 colons as scope. * c-c++-common/cilk-plus/AN/vla.c: Make this test C specific. * g++.dg/cilk-plus/AN/array_test1_tplt.cc: New test. * g++.dg/cilk-plus/AN/array_test2_tplt.cc: Likewise. * g++.dg/cilk-plus/AN/array_test_ND_tplt.cc: Likewise. * g++.dg/cilk-plus/AN/braced_list.cc: Likewise. * g++.dg/cilk-plus/AN/builtin_fn_custom_tplt.cc: Likewise. * g++.dg/cilk-plus/AN/builtin_fn_mutating_tplt.cc: Likewise. * g++.dg/cilk-plus/AN/fp_triplet_values_tplt.c: Likewise. * g++.dg/cilk-plus/cilk-plus.exp: New script. * gcc/testsuite/g++.dg/dg.exp: Included Cilk Plus C++ tests in the list. Thanks, Balaji V. Iyer. > -----Original Message----- > From: Iyer, Balaji V > Sent: Wednesday, June 12, 2013 4:50 PM > To: 'Aldy Hernandez' > Cc: gcc-patches@gcc.gnu.org; Jason Merrill (jason@redhat.com); > rth@redhat.com > Subject: RE: [PATCH] Cilk Plus Array Notation for C++ > > Hi Aldy, > Please see my comments below with the fixed patch. > > Thanks, > > Balaji V. Iyer. > > > -----Original Message----- > > From: Aldy Hernandez [mailto:aldyh@redhat.com] > > Sent: Wednesday, June 12, 2013 12:34 PM > > To: Iyer, Balaji V > > Cc: gcc-patches@gcc.gnu.org; Jason Merrill (jason@redhat.com); > > rth@redhat.com > > Subject: Re: [PATCH] Cilk Plus Array Notation for C++ > > > > [Jason/Richard: there are some things below I could use your feedback > > on.] > > > > Hi Balaji. > > > > Overall, a lot of the stuff in cp-array-notation.c looks familiar from > > the C front- end changes. Can't you reuse a lot of it? > > > > Otherwise, here are some minor nits... > > > > > + /* If the function call is builtin array notation function then we do not > > > + need to do any type conversion. */ > > > + if (flag_enable_cilkplus && fn && TREE_CODE (fn) == FUNCTION_DECL > > > + && DECL_NAME (fn) && IDENTIFIER_POINTER (DECL_NAME (fn)) > > > + && !strncmp (IDENTIFIER_POINTER (DECL_NAME (fn)), > > "__sec_reduce", 12)) > > > + val = arg; > > > > Don't we have BUILT_IN_CILKPLUS_SEC_REDUCE* now? So you shouldn't > > need to poke at the actual identifier. And even so, won't the above > > strncmp match __sec_reducegarbage? > > FIXED! > > > > > > +/* This function parses Cilk Plus array notations. The starting index is > > > + passed in INIT_INDEX and the array name is passed in ARRAY_VALUE. The > > > + return value of this function is a tree node called VALUE_TREE of type > > > + ARRAY_NOTATION_REF. If some error occurred it returns > > > +error_mark_node. */ > > > + > > > > It looks like a NULL in INIT_INDEX is a specially handled case. > > Perhaps you should document that INIT_INDEX can be null and what it means. > > Also, you don't need to document what internal variable name you are > > using as a return value (VALUE_TREE). Perhaps instead of "The return > > value..." you could write "This function returns the > > ARRAY_NOTATION_REF node." or something like it. > > It is documented inside the function, right before checking for !init_index. Is that > enough? > > > > > > + case ARRAY_NOTATION_REF: > > > + { > > > + tree start_index, length, stride; > > > + op1 = tsubst_non_call_postfix_expression (ARRAY_NOTATION_ARRAY > > (t), > > > + args, complain, in_decl); > > > + start_index = RECUR (ARRAY_NOTATION_START (t)); > > > + length = RECUR (ARRAY_NOTATION_LENGTH (t)); > > > + stride = RECUR (ARRAY_NOTATION_STRIDE (t)); > > > + > > > + /* We do type-checking here for templatized array notation triplets. */ > > > + if (!TREE_TYPE (start_index) > > > + || !INTEGRAL_TYPE_P (TREE_TYPE (start_index))) > > > + { > > > + error_at (loc, "start-index of array notation triplet is not an " > > > + "integer"); > > > + RETURN (error_mark_node); > > > + } > > > + if (!TREE_TYPE (length) || !INTEGRAL_TYPE_P (TREE_TYPE (length))) > > > + { > > > + error_at (loc, "length of array notation triplet is not an " > > > + "integer"); > > > + RETURN (error_mark_node); > > > + } > > > + if (!TREE_TYPE (stride) || !INTEGRAL_TYPE_P (TREE_TYPE (stride))) > > > + { > > > + error_at (loc, "stride of array notation triplet is not an " > > > + "integer"); > > > + RETURN (error_mark_node); > > > + } > > > + if (TREE_CODE (TREE_TYPE (op1)) == FUNCTION_TYPE) > > > + { > > > + error_at (loc, "array notations cannot be used with function type"); > > > + RETURN (error_mark_node); > > > + } > > > + RETURN (build_array_notation_ref (EXPR_LOCATION (t), op1, > > start_index, > > > + length, stride, TREE_TYPE (op1))); > > > + } > > > > > > You do all this type checking here, but aren't you doing the same type > > checking in build_array_notation_ref() which you're going to call > > anyway? It looks like there is some code duplication going on. > > > > Also, I see you have a build_array_notation_ref() in > > cp/cp-array-notation.c and also in c/c-array-notation.c. Can you not > > implement one function that handles both C and C++, or at the very least reuse > some of the common things? > > As we discussed in the previous email-exchange, I have created a new function > called cilkplus_an_triplet_types_ok_p (). The reason why I prefixed "cilkplus_an" > is because it is not a static function and I want to make sure this is unique and > descriptive and does not cause any interference with anything present or future. > > > > > You are missing a ChangeLog entry for the above snippet. > > I have put it in now. > > > > > > + /* If the return expr. has a builtin array notation function, then its > > > + OK. */ > > > + if (rank >= 1) > > > + { > > > + error_at (input_location, "array notation expression cannot be " > > > + "used as a return value"); > > > + return error_mark_node; > > > + } > > > > The comment doesn't seem to match the code, or am I missing something? > > > > > + /* If find_rank returns false, then it should have reported > > > + an error, > > FIXED > > > > > Extra whitespace. > > > > > + if (rank > 1) > > > + { > > > + error_at (loc, "rank of the array%'s index is greater than 1"); > > > + return error_mark_node; > > > + } > > > > No corresponding test. > > This is being tested in the file "testsuite/c-c++-common/cilk-plus/AN/gather- > scatter-errors.c" > > > > > > + /* If we are dealing with built-in array notation function then we don't > need > > > + to convert them. They will be broken up into modify exprs in future, > > > + during which all these checks will be done. */ > > > > Line too long, please wrap. > > > > There are various lines throughout your patch that are pretty long > > (both in code and in ChangeLog entries). I don't know what the > > official GNU guidelines say, but what I usually see as prior art in > > the GCC code base is something along the lines of wrapping around > > column 72. Perhaps someone can pontificate on this, but lines reaching the > 78-80 columns look pretty darn long to me. > > The line length is specified to be at most 80 characters > (http://gcc.gnu.org/codingconventions.html#Line). I have followed it > everywhere except in the cases of dg-error since I can't fit them all in one line. > > > > > diff --git gcc/testsuite/c-c++-common/cilk-plus/AN/sec_implicit_ex.c > > > gcc/testsuite/c-c++-common/cilk-plus/AN/sec_implicit_ex.c > > > index c22b818..b863276 100644 > > > --- gcc/testsuite/c-c++-common/cilk-plus/AN/sec_implicit_ex.c > > > +++ gcc/testsuite/c-c++-common/cilk-plus/AN/sec_implicit_ex.c > > > @@ -1,9 +1,6 @@ > > > /* { dg-do run } */ > > > /* { dg-options "-fcilkplus" } */ > > > > > > -void abort (void); > > > -void exit (int); > > > - > > > > > > int main(void) > > > { > > > @@ -24,10 +21,7 @@ int main(void) > > > for (jj = 0; jj < 10; jj++) > > > for (kk = 0; kk < 10; kk++) > > > if (array_3[ii][jj][kk] != array_3C[ii][jj][kk]) > > > - abort (); > > > + return 1; > > > > > > - > > > - exit (0); > > > - > > > return 0; > > > > Changes to existing tests should be submitted as a separate patch, > > since this doesn't seem to be C++ specific. And BTW, this particular > > test change can be committed as obvious. > > OK will do. What about adding {target c} and {target c++} for test cases. Can > that be submitted with the patch? > > > > > > + int iarray[10], iarray2[10], i_result, i_max; > > > + long larray[10], larray2[10], l_result, l_max; > > > + float farray[10], farray2[10], f_result, f_max; double > > > + darray[10], darray2[10], d_result, d_max; #if 1 for (int ii = 0; > > > + ii < 10; ii++) > > > + { > > > + if (ii%2 && ii) > > > > Remove this redundant #if 1/#endif pair. Similarly in other tests. > > FIXED! > > > > > > + if (flag_enable_cilkplus > > > + && contains_array_notation_expr (condition)) > > > + { > > > + error_at (EXPR_LOCATION (condition), > > > + "array notations cannot be used as a condition for " > > > + "switch statement"); > > > > No corresponding test, or is this handled by the existing > > c-c++-common/cilk-plus tests? > > Please see c-c++-common/cilk-plus/AN/misc.c > > > > > > + /* This could be a function ptr. If so, then emit error. */ > > > + subtype = TREE_TYPE (subtype); > > > + if (subtype && TREE_CODE (subtype) == FUNCTION_TYPE) > > > + { > > > + error_at (loc, "array notations cannot be used with" > > > + " function pointer arrays"); > > > > No need to document the obvious. The error message can be used in > > lieu of the comment :). Also, no corresponding test. I didn't see > > one matching in c-c++- common/cilkplus either. > > OK. This testcase (testsuite/c-c++-common/cilk-plus/AN/fn_ptr.c) checks this > error. > > > > > > + /* Disable correcting single colon correcting to scope. */ > > > + parser->colon_corrects_to_scope_p = false; > > > > No need to document the obvious. > > FIXED! > > > > > I suggest a different name for fix_array_notation_exprs() to avoid > > confusion with the C function fix_array_notation_expr() (no final "s"). > > Perhaps cpp_fix_array_notation_expr, or something similar? > > > > > +/* Handles expression of the form "a[i:j:k]" or "a[:]" or "a[i:j]," which > > > + denotes an array notation expression. If a is a variable or a > > > +member, then > > > > Do you mean "ARRAY" instead of "a"? > > > > > + we generate a ARRAY_NOTATION_REF front-end tree and return it. > > > > s/a ARRAY/an ARRAY/ > > > > > + This tree is broken down to ARRAY_REF toward the end of parsing. > > > + ARRAY_NOTATION_REF tree holds the START_INDEX, LENGTH, STRIDE > > > + and > > the TYPE > > > + of ARRAY_REF. Restrictions on START_INDEX, LENGTH and STRIDE is > > > + same > > as that > > > + of the index field passed into ARRAY_REF. The only additional restriction > > > + is that, unlike index in ARRAY_REF, stride, length and start_index cannot > > > + contain ARRAY_NOTATIONS. */ > > > + > > > +tree > > > +build_array_notation_ref (location_t loc, tree array, tree start_index, > > > + tree length, tree stride, tree type) > > > > Overall this function comment needs to be reworded. From reading the > > comment, I still don't know what the function actually does and what it > returns. > > "Handles expression..." is rather ambiguous. Perhaps something like > > "Given a blah blah blah in ARRAY, construct an ARRAY_NOTATION_REF and > return it. > > START_INDEX is blah, LENGTH is blah, etc etc". > > Reworded it. Please let me know if it is OK. > > > > > Again, you can probably reuse a lot of the C counterpart. It looks > > like a lot of the same code. > > > > > + XDELETEVEC (compare_expr); > > > + XDELETEVEC (expr_incr); > > > + XDELETEVEC (ind_init); > > > + XDELETEVEC (array_var); > > > + > > > + for (ii = 0; ii < list_size; ii++) > > > + { > > > + XDELETEVEC (count_down[ii]); > > > + XDELETEVEC (array_value[ii]); > > > + XDELETEVEC (array_stride[ii]); > > > + XDELETEVEC (array_length[ii]); > > > + XDELETEVEC (array_start[ii]); > > > + XDELETEVEC (array_ops[ii]); > > > + XDELETEVEC (array_vector[ii]); > > > + } > > > + > > > + XDELETEVEC (count_down); > > > + XDELETEVEC (array_value); > > > + XDELETEVEC (array_stride); > > > + XDELETEVEC (array_length); > > > + XDELETEVEC (array_start); > > > + XDELETEVEC (array_ops); > > > + XDELETEVEC (array_vector); > > > > I see a lot of this business going on. Perhaps one of the core > > maintainers can comment, but I would rather use an obstack, and avoid > > having to keep track of all these little buckets-- which seems rather > > error prone, and then free the obstack all in one swoop. But I'll defer to > Richard or Jason. > > > > > > > + is not, we convert induction variable to stride's > > > > Rephrase as "is not, convert the induction variable to the stride's" > > Similarly in a few other places. It looks like you used the same comment. > > FIXED! > > > > > > + /* If we reach here, then the stride and start are of > > > + different types, and so it doesn't really matter what > > > + the induction variable type is, we stay safe and convert > > > > s/type is, we stay safe/type is. We stay safe/. > > Similarly in a few other places. It looks like you used the same comment. > > FIXED! > > > > > > + everything to integer. The reason why we pick integer > > > > s/pick integer/pick an integer > > Similarly in a few other places. > > FIXED! > > > > > > +/* Returns a loop with ARRAY_REF inside it with an appropriate modify > expr. > > > + The LHS and/or RHS will be array notation expressions that have a > > > + MODIFYCODE. The location of the variable is specified by > > > +LOCATION. */ > > > + > > > +static tree > > > +build_x_array_notation_expr (location_t location, tree lhs, > > > + enum tree_code modifycode, tree rhs, > > > + tsubst_flags_t complain) > > > > This is called "build_x_array_notation_expr", but it doesn't look like > > we build any ARRAY_NOTATION_EXPRs in here. Perhaps a better name > > would be "expand_array_notation_expr" (or something to that effect)? > > I renamed this function to "expand_an_in_modify_expr" to indicate that we are > expanding array notations in modify expr. > > > > > > +static tree > > > +fix_builtin_array_notation_fn (tree an_builtin_fn, tree *new_var) > > > > It looks like this function expands the __sec_reduce* builtins. > > Perhaps it would be cleaner to rename it as such? Maybe... > > "expand_sec_reduce_builtin" or something? > > FIXED! > > > > > The fact that you are sometimes using "build_*" and sometimes "fix_*" > > to denote expansion is confusing. > > Replaced all the "fix" with "expand" > > > > > > +/* Returns true of NODE has a call_expression with > > > +ARRAY_NOTATION_REF tree. */ > > > + > > > +static bool > > > +has_call_expr_with_array_notation (tree node) > > > > s/of NODE/if NODE > > FIXED! > > > > > Thanks. > > Aldy
Attachment:
patch_array_notation_cpp.txt
Description: patch_array_notation_cpp.txt
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |