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? +/* 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. +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? You are missing a ChangeLog entry for the above snippet. + /* 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, 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. + /* 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. diff --git gcc/testsuite/c-c++-common/cilk-plus/AN/sec_implicit_ex.c gcc/testsuite/c-c++-common/cilk-plus/AN/sec_implicit_
RE: [PATCH] Cilk Plus Array Notation for C++
Hi Aldy, Below are my responses to a couple of the things you pointed out. Thanks, Balaji V. Iyer. > -Original Message- > From: Aldy Hernandez [mailto:al...@redhat.com] > Sent: Wednesday, June 12, 2013 12:34 PM > To: Iyer, Balaji V > Cc: gcc-patches@gcc.gnu.org; Jason Merrill (ja...@redhat.com); > r...@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? I looked into trying to combine many functionality. The issue that prohibited me was templates and extra trees. For example, IF_STMT, FOR_STMT, MODOP_EXPR, etc are not available in C but are in C++. So, I had to add this additional check for those. Also, if we are processing templates we have to create different kind of trees (e.g MODOP_EXPR intead of MODIFY_EXPR). One way to do it is to break up the places where I am using C++ specific code and add a language hook to handle those. I tried doing that a while back and the whole thing looked a lot messy and I would imagine it would be hard to debug them in future (...atleast for me). This looked organized for me, even though a few code looks repeated. Also, the function names are repeated because they do similar things in C and C++ only thing is that the body of the function is different. > > > +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. The reason why we do this second type checking here is because we don't know what they could be when we are parsing it. For example, in: T x,y,z A[x:y:z] x, y, z could be floats and that should be flagged as error, but if x, y z are ints, then its ok. We don't know this information until we hit this spot in pt.c > > 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? I looked into that also, but templates got in the way. > > You are missing a ChangeLog entry for the above snippet. That I will put in. > > > + 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_vec
Re: [PATCH] Cilk Plus Array Notation for C++
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? I looked into trying to combine many functionality. The issue that prohibited me was templates and extra trees. For example, IF_STMT, FOR_STMT, MODOP_EXPR, etc are not available in C but are in C++. So, I had to add this additional check for those. Also, if we are processing templates we have to create different kind of trees (e.g MODOP_EXPR intead of MODIFY_EXPR). I see. One way to do it is to break up the places where I am using C++ specific code and add a language hook to handle those. I tried doing that a while back and the whole thing looked a lot messy and I would imagine it would be hard to debug them in future (...atleast for me). This looked organized for me, even though a few code looks repeated. That's what I had in mind, but if you tried it and it looks worse, I guess I can live with it. 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. The reason why we do this second type checking here is because we don't know what they could be when we are parsing it. For example, in: Couldn't you abstract the type checking out into a helper function shared by both routines? 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? I looked into that also, but templates got in the way. Ughh... ok, I'll let Jason deal with this then. + 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. They are temporary variables that are used to store information necessary for expansion. To me, dynamic arrays seem to be the most straight-forward way to do it. Changing them would involve pretty much rewriting the whole thing and thus maybe breaking the stability. So, if it is not a huge issue, I would like to keep the dynamic arrays. They are not being used anywhere else just inside the function. This is not huge, so don't worry, but XNEWVEC is just a wrapper to xmalloc (see include/libiberty.h). You could do the exact thing with XOBNEWVEC and save yourself all the XDELETEVECs, with one obstack_free().
RE: [PATCH] Cilk Plus Array Notation for C++
> -Original Message- > From: Aldy Hernandez [mailto:al...@redhat.com] > Sent: Wednesday, June 12, 2013 1:40 PM > To: Iyer, Balaji V > Cc: gcc-patches@gcc.gnu.org; Jason Merrill (ja...@redhat.com); > r...@redhat.com > Subject: Re: [PATCH] Cilk Plus Array Notation for C++ > > > >> 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? > > > > I looked into trying to combine many functionality. The issue that > > prohibited me was templates and extra trees. For example, IF_STMT, > > FOR_STMT, MODOP_EXPR, etc are not available in C but are in C++. So, I > > had to add this additional check for those. Also, if we are processing > > templates we have to create different kind of trees (e.g MODOP_EXPR > > intead of MODIFY_EXPR). > > I see. > > > > > One way to do it is to break up the places where I am using C++ > > specific code and add a language hook to handle those. I tried doing > > that a while back and the whole thing looked a lot messy and I would > > imagine it would be hard to debug them in future (...atleast for me). > > This looked organized for me, even though a few code looks repeated. > > That's what I had in mind, but if you tried it and it looks worse, I guess I > can live > with it. > > >> 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. > > > > The reason why we do this second type checking here is because we > > don't know what they could be when we are parsing it. For example, > > in: > > Couldn't you abstract the type checking out into a helper function shared by > both routines? Yes, that I could do. I will fix it in the new upcoming Array Notation for C++ patch. Thanks, Balaji V. Iyer.
Re: [PATCH] Cilk Plus Array Notation for C++
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? Please put it in the function comment. As it stands, users would have to look through the body to find that init_index==NULL is a special case. 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? Yes. + else if (an_type == BUILT_IN_CILKPLUS_SEC_REDUCE_MAX) +{ + /* If the TYPE_MIN_VALUE is available for the new_var_type, then +set that as the initial value. */ + if (TYPE_MIN_VALUE (new_var_type)) + new_var_init = build_x_modify_expr (location, *new_var, NOP_EXPR, + TYPE_MIN_VALUE (new_var_type), 1); + else + /* ... otherwise set initial value as the first element of array. */ + new_var_init = build_x_modify_expr (location, *new_var, NOP_EXPR, + func_parm, 1); + new_no_expr = build_x_modify_expr (location, *new_var, NOP_EXPR, + *new_var, 1); + new_yes_expr = build_x_modify_expr (location, *new_var, NOP_EXPR, + func_parm, 1); + new_cond_expr = build_x_binary_op (location, LT_EXPR, *new_var, +TREE_CODE (*new_var), func_parm, +TREE_CODE (func_parm), NULL, +tf_warning_or_error); + new_expr = build_x_conditional_expr (location, new_cond_expr, + new_yes_expr, new_no_expr, + tf_warning_or_error); +} + else if (an_type == BUILT_IN_CILKPLUS_SEC_REDUCE_MIN) +{ + /* If the TYPE_MAX_VALUE is available for the new_var_type, then +set that as the initial value. */ + if (TYPE_MAX_VALUE (new_var_type)) + new_var_init = build_x_modify_expr (location, *new_var, NOP_EXPR, + TYPE_MAX_VALUE (new_var_type), 1); + else + /* ... otherwise set initial value as the first element of array. */ + new_var_init = build_x_modify_expr (location, *new_var, NOP_EXPR, + func_parm, 1); + new_no_expr = build_x_modify_expr (location, *new_var, NOP_EXPR, + *new_var, 1); + new_yes_expr = build_x_modify_expr (location, *new_var, NOP_EXPR, + func_parm, 1); + new_cond_expr = build_x_binary_op (location, GT_EXPR, *new_var, +TREE_CODE (*new_var), func_parm, +TREE_CODE (func_parm), NULL, +tf_warning_or_error); + new_expr = build_x_conditional_expr (location, new_cond_expr, + new_yes_expr, new_no_expr, + tf_warning_or_error); +} The whole slew of these cases have a lot of duplicated code. For instance, BUILT_IN_CILKPLUS_SEC_REDUCE_MIN is the same as BUILT_IN_CILKPLUS_SEC_REDUCE_MAX, the only difference being GT_EXPR vs LT_EXPR. Surely you could do something like: if (an_type == BUILT_IN_CILKPLUS_SEC_REDUCE_MIN || an_type == BUILT_IN_CILKPLUS_SEC_REDUCE_MAX) { enum tree_code code; if (an_type == BUILT_IN_CILKPLUS_SEC_REDUCE_MIN) code = GT_EXPR; else code = LT_EXPR; // stuff } The compiler should be able to optimize the above, but even if it couldn't, I am willing to compare twice and save lines and lines of code. Similarly for SEC_REDUCE_ANY_ZERO/SEC_REDUCE_ANY_NONZERO, SEC_REDUCE_ALL_ZERO/SEC_REDUCE_ALL_NONZERO, SEC_REDUCE_ADD/SEC_REDUCE_MUL, etc etc etc. + if (location == UNKNOWN_LOCATION) +{ + if (EXPR_LOCATION (lhs) != UNKNOWN_LOCATION) + location = EXPR_LOCATION (lhs); + else if (EXPR_LOCATION (rhs) != UNKNOWN_LOCATION) + location = EXPR_LOCATION (rhs); +} + + + /* We need this when we have a scatter issue. */ Extra whitespace. + if (lhs_rank != 0 && rhs_rank != 0 && lhs_rank != rhs_rank) +{ + tree lhs_base = lhs; + tree rhs_base = rhs; + + for (ii = 0; ii < lhs_rank; ii++) + lhs_base = ARRAY_NOTATION_ARRAY (lhs_base); + + while (rhs_base && TREE_CODE (rhs_base)
Re: [PATCH] Cilk Plus Array Notation for C++
On 06/13/2013 09:11 AM, Aldy Hernandez wrote: > The whole slew of these cases have a lot of duplicated code. For instance, > BUILT_IN_CILKPLUS_SEC_REDUCE_MIN is the same as > BUILT_IN_CILKPLUS_SEC_REDUCE_MAX, the only difference being GT_EXPR vs > LT_EXPR. Surely you could do something like: > > if (an_type == BUILT_IN_CILKPLUS_SEC_REDUCE_MIN > || an_type == BUILT_IN_CILKPLUS_SEC_REDUCE_MAX) { >enum tree_code code; >if (an_type == BUILT_IN_CILKPLUS_SEC_REDUCE_MIN) > code = GT_EXPR; >else > code = LT_EXPR; >// stuff > } > > The compiler should be able to optimize the above, but even if it couldn't, I > am willing to compare twice and save lines and lines of code. > > Similarly for SEC_REDUCE_ANY_ZERO/SEC_REDUCE_ANY_NONZERO, > SEC_REDUCE_ALL_ZERO/SEC_REDUCE_ALL_NONZERO, SEC_REDUCE_ADD/SEC_REDUCE_MUL, etc > etc etc. Yep. It's at this point that I normally start using the idiom switch (an_type) { case BUILT_IN_CILKPLUS_SEC_REDUCE_MIN: code = GT_EXPR; goto do_min_max; case BUILT_IN_CILKPLUS_SEC_REDUCE_MAX: code = LT_EXPR; goto do_min_max; do_min_max: // stuff So much the better if you can include the other SEC_* cases in that switch too, doing one compiler-controlled dispatch. It also occurs to me to wonder why you're building your own COND_EXPR here, with the comparison, rather than using MIN/MAX_EXPR... r~
RE: [PATCH] Cilk Plus Array Notation for C++
> -Original Message- > From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches- > ow...@gcc.gnu.org] On Behalf Of Richard Henderson > Sent: Thursday, June 13, 2013 12:19 PM > To: Aldy Hernandez > Cc: Iyer, Balaji V; gcc-patches@gcc.gnu.org; Jason Merrill (ja...@redhat.com) > Subject: Re: [PATCH] Cilk Plus Array Notation for C++ > > On 06/13/2013 09:11 AM, Aldy Hernandez wrote: > > The whole slew of these cases have a lot of duplicated code. For > > instance, BUILT_IN_CILKPLUS_SEC_REDUCE_MIN is the same as > > BUILT_IN_CILKPLUS_SEC_REDUCE_MAX, the only difference being GT_EXPR > vs > > LT_EXPR. Surely you could do something like: > > > > if (an_type == BUILT_IN_CILKPLUS_SEC_REDUCE_MIN > > || an_type == BUILT_IN_CILKPLUS_SEC_REDUCE_MAX) { > >enum tree_code code; > >if (an_type == BUILT_IN_CILKPLUS_SEC_REDUCE_MIN) > > code = GT_EXPR; > >else > > code = LT_EXPR; > >// stuff > > } > > > > The compiler should be able to optimize the above, but even if it > > couldn't, I am willing to compare twice and save lines and lines of code. > > > > Similarly for SEC_REDUCE_ANY_ZERO/SEC_REDUCE_ANY_NONZERO, > > SEC_REDUCE_ALL_ZERO/SEC_REDUCE_ALL_NONZERO, > > SEC_REDUCE_ADD/SEC_REDUCE_MUL, etc etc etc. > > Yep. It's at this point that I normally start using the idiom > > switch (an_type) > { > case BUILT_IN_CILKPLUS_SEC_REDUCE_MIN: > code = GT_EXPR; > goto do_min_max; > case BUILT_IN_CILKPLUS_SEC_REDUCE_MAX: > code = LT_EXPR; > goto do_min_max; > do_min_max: > // stuff > > So much the better if you can include the other SEC_* cases in that switch > too, > doing one compiler-controlled dispatch. I didn't use goto, but I did try to abstract out the common parts and either tried to lump them together in the same case or put them outside of the case. The new patch is here: http://gcc.gnu.org/ml/gcc-patches/2013-06/msg00984.html > > It also occurs to me to wonder why you're building your own COND_EXPR here, > with the comparison, rather than using MIN/MAX_EXPR... In hindsight, I could have for __sec_reduce_max and __sec_reduce_min. I was more familiar with conditional expression. Out of curiosity, is there a big performance benefit of using max/min expr over conditional? Thanks, Balaji V. Iyer. > > > r~
Re: [PATCH] Cilk Plus Array Notation for C++
On 06/17/2013 10:00 AM, Iyer, Balaji V wrote: > In hindsight, I could have for __sec_reduce_max and __sec_reduce_min. I was > more familiar with conditional expression. Out of curiosity, is there a big > performance benefit of using max/min expr over conditional? There can be. The COND->MIN/MAX transformation is not done without the -ffinite-math-only component of -ffast-math. I.e. we don't try the transform when NaNs are a possibility. So, yes, you probably should generate MIN/MAX_EXPR right from the start. r~
Re: [PATCH] Cilk Plus Array Notation for C++
Thanks for fixing everything. This looks much better. I don't have much, just small typos. It's up to Jason and Richard to take it from here. Aldy + else + { + val = convert_like_with_context (conv, arg, fn, i-is_method, + conversion_warning + ? complain + : complain & (~tf_warning)); s/i-is_method/i - is_method/ I know this isn't your fault, since the original code had this, but if you're going to copy over, might as well fix small formatting errors :). +/* Extracts all the array notation triplet information from LIST and stores them + in a 2-D arrays (size x rank) of START, LENGTH and STRIDE, holding the s/2-D arrays/2-D array/ + For example: For an array notation A[5:10:2], the vector start will be Extra space after start. + /* We need to do this because we are "faking" the builtin function types, +so the compiler does a bunch of typecasts and this will get rid of +all that! */ + while (TREE_CODE (call_fn) == CONVERT_EXPR +|| TREE_CODE (call_fn) == NOP_EXPR) + call_fn = TREE_OPERAND (call_fn, 0); We already have an automated way of doing this. Will this work for you?: /* Given an expression as a tree, strip any conversion that generates no instruction. Accepts both tree and const_tree arguments since we are not modifying the tree itself. */ #define STRIP_NOPS(EXP) \ (EXP) = tree_strip_nop_conversions (CONST_CAST_TREE (EXP)) Similarly elsewhere. +/* Helper function for expand_conditonal_array_notations. Encloses the Two spaces after "." +static tree +expand_return_expr (tree expr) +{ + tree new_mod_list, new_var, new_mod, retval_expr; + location_t loc = EXPR_LOCATION (expr); + + if (TREE_CODE (expr) != RETURN_EXPR) +return expr; Well, since we're using C++, might as well move the comparison with RETURN_EXPR earlier, to avoid setting loc. + /* Skip empty subtrees. */ + if (!t) +return t; No need to document the obvious. +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)); + if (!cilkplus_an_triplet_types_ok_p (loc, start_index, length, stride, +TREE_TYPE (op1))) + RETURN (error_mark_node); + RETURN (build_array_notation_ref (EXPR_LOCATION (t), op1, start_index, + length, stride, TREE_TYPE (op1))); Indentation of RETURN is wrong. + /* If the return expression contains array notatinos, then flag it as s/notatinos/notations. Though I like the sound of "notatinos" :-). + /* If find_rank returns false, then it should have reported an error, s/ then/then + to convert them. They will be broken up into modify exprs in future, Two spaces after period.
Re: [PATCH] Cilk Plus Array Notation for C++
> +/* Returns true if there is a length mismatch among exprssions that are at > the > + same dimension and one the same side of the equal sign. The Array > notation > + lengths (LIST) is passed in as a 2D vector of trees. */ > + > +static bool > +cp_length_mismatch_in_expr_p (location_t loc, vec >list) Other than working on a vec<>, how does this differ from the length_mismatch_in_expr_p (location_t loc, tree **list, size_t x, size_t y) in c-family? > +static inline void > +clear_all_an_vectors (vec > *value, vec > *start, > + vec > *length, vec > *stride, > + vec > *is_vector, vec > *count_down, > + vec *incr, vec *cmp, vec *ind_init, > + vec *var) > +{ > + value->release (); > + start->release (); > + length->release (); > + stride->release (); > + is_vector->release (); > + count_down->release (); > + incr->release (); > + cmp->release (); > + ind_init->release (); > + var->release (); > +} 10 arrays kept in sync? It's at this point that we should realize that there's a mistake in how we're arranging the data. This should be one array, with the element being a structure. That should significantly improve quite a lot of the functions in this file. > + init = ARITHMETIC_TYPE_P (new_var_type) ? build_zero_cst (new_var_type) > + : integer_zero_node; Why the test for ARITHMETIC_TYPE_P? Surely we always want something of new_var_type. > + if (an_type == BUILT_IN_CILKPLUS_SEC_REDUCE_ADD > + || an_type == BUILT_IN_CILKPLUS_SEC_REDUCE_MUL) > +new_expr = build_x_modify_expr (location, *new_var, code, func_parm, > + tf_warning_or_error); > + if (an_type == BUILT_IN_CILKPLUS_SEC_REDUCE_ALL_ZERO > + || an_type == BUILT_IN_CILKPLUS_SEC_REDUCE_ALL_NONZERO > + || an_type == BUILT_IN_CILKPLUS_SEC_REDUCE_ANY_ZERO > + || an_type == BUILT_IN_CILKPLUS_SEC_REDUCE_ANY_NONZERO) Why not another switch statement here? r~
Re: [PATCH] Cilk Plus Array Notation for C++
On 06/20/2013 04:39 PM, Iyer, Balaji V wrote: > I couldn't put them into 1 structure, so I made 2 structures holding the > following information: array notation triplet information and array notation > expansion loop's information. It is fixed in the patch attached. Excellent, thanks. One thing that seems to be missed in this conversion is that .count_down is never set, only read. Mistake in +cilkplus_extract_an_triplets? Otherwise this is looking good. r~
RE: [PATCH] Cilk Plus Array Notation for C++
> -Original Message- > From: Richard Henderson [mailto:r...@redhat.com] > Sent: Friday, June 21, 2013 12:11 PM > To: Iyer, Balaji V > Cc: Aldy Hernandez; gcc-patches@gcc.gnu.org; Jason Merrill > (ja...@redhat.com) > Subject: Re: [PATCH] Cilk Plus Array Notation for C++ > > On 06/20/2013 04:39 PM, Iyer, Balaji V wrote: > > I couldn't put them into 1 structure, so I made 2 structures holding > > the following information: array notation triplet information and > > array notation expansion loop's information. It is fixed in the patch > > attached. > > Excellent, thanks. One thing that seems to be missed in this conversion is > that > .count_down is never set, only read. Mistake in > +cilkplus_extract_an_triplets? Count down came from an initial implementation were if the stride was negative, I set that bool to true and then we count down. But, the way it stands now, I can remove the count down field. I can remove it. After I remove that field, will it be OK for trunk? Thanks, Balaji V. Iyer. > > Otherwise this is looking good. > > > r~
Re: [PATCH] Cilk Plus Array Notation for C++
On 06/21/2013 09:59 AM, Iyer, Balaji V wrote: > After I remove that field, will it be OK for trunk? Yes. r~
Re: [PATCH] Cilk Plus Array Notation for C++
Hmm, seems like I should have sent this yesterday even though I hadn't made it through the whole patch. But I suppose it doesn't hurt to fix it after checkin. On 06/20/2013 07:39 PM, Iyer, Balaji V wrote: diff --git a/gcc/c-family/ChangeLog b/gcc/c-family/ChangeLog old mode 100644 new mode 100755 index a0f195b..fb70520 Binary files a/gcc/c-family/ChangeLog and b/gcc/c-family/ChangeLog differ Why are you marking lots of files as executable? In the future please filter out ChangeLogs entirely from diffs. I use filterdiff -x '*/ChangeLog' | sed -e '/^diff.*ChangeLog/{N;d}' for my own patches. + if (flag_enable_cilkplus + && (contains_array_notation_expr (expr) + || contains_array_notation_expr (fn))) Looking at "fn" here doesn't make sense; it's only used for diagnostics. + /* If we are using array notations, we fix them up at a later stage + and we will do these checks then. */ + ; And please don't mess with the overload resolution code directly to handle this case differently. This seems to be a general problem with the patch; you are special-casing array notation all through the compiler rather than making it work with the existing mechanisms. In this case, an ARRAY_NOTATION_REF should have a type that participates normally with conversions. + /* If the function call is builtin array notation function then no need +to do any type conversion. */ And here, instead of changing build_over_call, you can use the existing magic_varargs_p mechanism. +/* This function parses Cilk Plus array notations. The starting index is + passed in INIT_INDEX and the array name is passed in ARRAY_VALUE. If the + INIT_INDEX is NULL, then we have special case were the entire array is "where" + accessed (e.g. A[:]). The return value of this function is a tree node + called VALUE_TREE of type ARRAY_NOTATION_REF. If some error occurred it Drop "called VALUE_TREE"; the function comment shouldn't talk about local variables. + cp_token *token = NULL; + tree start_index = NULL_TREE, length_index = NULL_TREE, stride = NULL_TREE; + tree value_tree, type, array_type, array_type_domain; + double_int x; + bool saved_colon_corrects_to_scope_p = parser->colon_corrects_to_scope_p; Now that the compiler is built as C++, variables should be defined at the point of first use, rather than at the top of the function. + if (TREE_CODE (array_type) == RECORD_TYPE + || TREE_CODE (array_type) == POINTER_TYPE) + { + error_at (loc, "start-index and length fields necessary for " + "using array notations in pointers or records"); I think this should handle all non-array types, rather than just pointers and classes. Let's say "notation" rather than "notations" in all diagnostics. + error_at (loc, "array notations cannot be used with" + " function pointer arrays"); I don't see this restriction in any of the documentation. What's the rationale? + error_at (loc, "start-index and length fields necessary for " + "using array notations in dimensionless arrays"); Let's say "...with array of unknown bound" + start_index = fold_build1 (CONVERT_EXPR, ptrdiff_type_node, +start_index); Use cp_fold_convert rather than fold_build1. + x = TREE_INT_CST (TYPE_MAXVAL (array_type_domain)); + x.low++; + length_index = double_int_to_tree (integer_type_node, x); This assumes a constant length array. Use size_binop instead. + stride = build_int_cst (integer_type_node, 1); + stride = fold_build1 (CONVERT_EXPR, ptrdiff_type_node, stride); Build the constant in ptrdiff_type_node rather than build it in integer_type_node and then convert. + /* Disable correcting single colon correcting to scope. */ + parser->colon_corrects_to_scope_p = false; + stride = cp_parser_expression (parser, false, NULL); + parser->colon_corrects_to_scope_p = + saved_colon_corrects_to_scope_p; Why do you do this for the stride? We've already seen both array-notation colons at this point. + /* We fold all 3 of the values to make things easier when we transform + them later. */ Why is this better than folding at transformation time? +/* If we are here, then we have something like this: + ARRAY[:] +*/ The */ should go at the end of the last line in all comments. - if (for_offsetof) -index = cp_parser_constant_expression (parser, false, NULL); ... else { - if (cp_lexer_next_token_is (parser->lexer, CPP_OPEN_BRACE)) ... + if (for_offsetof) + index = cp_parser_constant_expression (parser, false, NULL); + else Please try to avoid re-indenting code when possible, to make history annotation sim
Re: [PATCH] Cilk Plus Array Notation for C++
A few more comments: + if (processing_template_decl || !TREE_TYPE (t)) + new_var = build_min_nt_loc (EXPR_LOCATION (t), VAR_DECL, NULL_TREE, + NULL_TREE); Again, we shouldn't be trying to expand array notation during template parsing. And replace_invariant_exprs should also use get_temp_regvar, as should most of the places you create temporary variables. + /* We need to do this because we are "faking" the builtin function types, +so the compiler does a bunch of typecasts and this will get rid of +all that! */ Again, magic_varargs_p can avoid any gratuitous type conversions. +case BUILT_IN_CILKPLUS_SEC_REDUCE_ALL_ZERO: +case BUILT_IN_CILKPLUS_SEC_REDUCE_ANY_ZERO: +case BUILT_IN_CILKPLUS_SEC_REDUCE_ANY_NONZERO: +case BUILT_IN_CILKPLUS_SEC_REDUCE_ALL_NONZERO: + new_var_type = integer_type_node; I would expect boolean_type_node. + an_loop_info[ii].var = build_decl (location, VAR_DECL, NULL_TREE, + TREE_TYPE (an_info[0][ii].start)); Here you can use create_temporary_var. + an_loop_info[ii].ind_init = build_x_modify_expr + (location, an_loop_info[ii].var, NOP_EXPR, +build_zero_cst (TREE_TYPE (an_loop_info[ii].var)), +tf_warning_or_error); Here and in other calls to build_x_modify_expr for initialization, use INIT_EXPR rather than NOP_EXPR. + *new_var = create_tmp_var (new_var_type, NULL); create_tmp_var is part of gimplification; it might be appropriate to use it when you move lowering to happen later in the compilation, but it seems wrong in the current code. + code = (an_type == BUILT_IN_CILKPLUS_SEC_REDUCE_ANY_ZERO) ? EQ_EXPR + : NE_EXPR; A ?: expression split across multiple lines should have parens around it. + /* If both are scalar, then the only reason why we will get this far is if + there is some array notations inside it and was using a builtin array + notation functions. If so, we have already broken those guys up and now + a simple build_x_modify_expr would do. */ + if (lhs_rank == 0 && rhs_rank == 0) +{ + if (found_builtin_fn) + { + new_modify_expr = build_x_modify_expr (location, lhs, +modifycode, rhs, complain); + finish_expr_stmt (new_modify_expr); + pop_stmt_list (an_init); + return an_init; + } + else + { + pop_stmt_list (an_init); + return NULL_TREE; + } +} The comment makes it sound like the else should be gcc_unreachable. + if (location == UNKNOWN_LOCATION && EXPR_HAS_LOCATION (rhs)) + location = EXPR_LOCATION (rhs); This is redundant with code a few lines above. + append_to_statement_list_force (lhs_an_loop_info[ii].ind_init, + &init_list); Why do you need _force? Jason
Re: [PATCH] Cilk Plus Array Notation for C++
On 06/24/2013 06:23 PM, Iyer, Balaji V wrote: Actually, to reduce the amount of changes to non-AN code, let's put the AN case third, after the offset and {} cases, so you get something like else if (flag_enable_cilkplus) { tree an = cp_parser_array_notation (loc, parser, &index, postfix_expression); if (an) return an; /* Otherwise, cp_parser_array_notation set 'index'. */ } else index = cp_parser_expression (parser, /*cast_p=*/false, NULL); this way the change is just a few added lines, and everything else is in cp_parser_array_notation. If I do it this way, then I don't think I will be able to handle a normal array reference when cilk plus is enabled. What I had in mind is that in the case of a normal array reference, cp_parser_array_notation will update index (which is passed by address) and return NULL_TREE, so that it gets back on the normal path. One thing I could do is to assume that people won't use array notations in braced list. I had it like this a while back but I made the change to make sure I have covered more cases. Please let me know if that is OK and I will fix it. Yes, that's OK. I looked into this. But, magic_varargs_p is static to call.c. Should I make it non-static? Yes. After making it non-static I was planning to do something like this: if (magic_varargs_p (fndecl) Nargs = (*params)->length (); else nargs = convert_arguments (...) Is that OK with you? I was thinking to check magic_varargs_p in convert_arguments, where it currently has if (fndecl && DECL_BUILT_IN (fndecl) && DECL_FUNCTION_CODE (fndecl) == BUILT_IN_CONSTANT_P) /* Don't do ellipsis conversion for __built_in_constant_p as this will result in spurious errors for non-trivial types. */ change to "if (fndecl && magic_varargs_p (fndecl))" By the way, why are you breaking out the elements of the ARRAY_NOTATION_REF into a cilkplus_an_parts rather than using the _REF directly? I use the different parts of array notations for error checking and to create the outer for-loop. Also, to create the ARRAY_REF I need the induction variable. I understand the need for cilkplus_an_loop_parts, but cilkplus_an_parts seems to contain exactly the same information as the ARRAY_NOTATION_REF. Fixed! By the way, what is SFINAE? SFINAE stands for "substitution failure is not an error". During template argument deduction, once we have a full set of template arguments we try to substitute them into the template declaration. In that context, things that would normally cause an error just fail and return error_mark_node silently. diff --git a/gcc/cp/ChangeLog b/gcc/cp/ChangeLog index 55ed6a5..9d570b2 100644 Binary files a/gcc/cp/ChangeLog and b/gcc/cp/ChangeLog differ This patch still has ChangeLog entries. + new_var = get_temp_regvar (TREE_TYPE (retval_expr), +build_zero_cst (TREE_TYPE (retval_expr))); new_mod = expand_an_in_modify_expr (loc, new_var, NOP_EXPR, + TREE_OPERAND (retval_expr, 1), + tf_warning_or_error); Why not pass TREE_OPERAND (retval_expr, 1) as the init to get_temp_regvar? - parser->colon_corrects_to_scope_p = saved_colon_corrects_to_scope_p; if (!length_index || length_index == error_mark_node) cp_parser_skip_to_end_of_statement (parser); @@ -6180,7 +6158,6 @@ cp_parser_array_notation (location_t loc, cp_parser *parser, tree init_index, saved_colon_corrects_to_scope_p = parser->colon_corrects_to_scope_p; /* Disable correcting single colon correcting to scope. */ - parser->colon_corrects_to_scope_p = false; This change looks like now you will only restore parser->colon_corrects_to_scope_p if you have a stride. I would suggest putting back the first restore, and removing all the corrects_to_scope_p code from the stride block, since there can't be an array-notation colon after the stride. + /* If stride and start are of same type and the induction var + is not, convert induction variable to stride's type. */ + if (TREE_TYPE (start) == TREE_TYPE (stride) + && TREE_TYPE (stride) != TREE_TYPE (var)) This seems impossible, since 'var' is created to have the same type as 'start'. As you suggested further in the email, I have made var of type ptrdiff_type, so I think I should keep this. I think it would be better to convert start/stride to ptrdiff_t. Incidentally, types should be compared with same_type_p rather than ==. + if (lhs_list_size > 0 && rhs_list_size > 0 && lhs_rank > 0 && rhs_rank > 0 + && TREE_CODE (lhs_len) == INTEGER_CST && rhs_len + && TREE_CODE (rhs_len) == INTEGER_CST) +{ + HOST_WIDE_INT l_length = int_cst_value (lhs_len); + HOST_W
RE: [PATCH] Cilk Plus Array Notation for C++
> -Original Message- > From: Jason Merrill [mailto:ja...@redhat.com] > Sent: Tuesday, June 25, 2013 10:39 AM > To: Iyer, Balaji V; Richard Henderson > Cc: Aldy Hernandez; gcc-patches@gcc.gnu.org > Subject: Re: [PATCH] Cilk Plus Array Notation for C++ > > On 06/24/2013 06:23 PM, Iyer, Balaji V wrote: > >> Actually, to reduce the amount of changes to non-AN code, let's put > >> the AN case third, after the offset and {} cases, so you get > >> something like > >> > >> else if (flag_enable_cilkplus) > >>{ > >> tree an = cp_parser_array_notation (loc, parser, &index, > >>postfix_expression); > >> if (an) > >>return an; > >> /* Otherwise, cp_parser_array_notation set 'index'. */ > >>} > >> else > >>index = cp_parser_expression (parser, /*cast_p=*/false, NULL); > >> > >> this way the change is just a few added lines, and everything else is > >> in cp_parser_array_notation. > > > If I do it this way, then I don't think I will be able to handle a normal > > array > reference when cilk plus is enabled. > > What I had in mind is that in the case of a normal array reference, > cp_parser_array_notation will update index (which is passed by address) and > return NULL_TREE, so that it gets back on the normal path. > > > One thing I could do is to assume that people won't use array notations in > braced list. I had it like this a while back but I made the change to make > sure I > have covered more cases. Please let me know if that is OK and I will fix it. > > Yes, that's OK. > > > I looked into this. But, magic_varargs_p is static to call.c. Should I make > > it non- > static? > > Yes. > > > After making it non-static I was planning to do something like this: > > > > if (magic_varargs_p (fndecl) > > Nargs = (*params)->length (); > > else > >nargs = convert_arguments (...) > > > > Is that OK with you? > > I was thinking to check magic_varargs_p in convert_arguments, where it > currently has > > > if (fndecl && DECL_BUILT_IN (fndecl) > > && DECL_FUNCTION_CODE (fndecl) == BUILT_IN_CONSTANT_P) > > /* Don't do ellipsis conversion for __built_in_constant_p > >as this will result in spurious errors for non-trivial > >types. */ > > change to "if (fndecl && magic_varargs_p (fndecl))" > This change is implemented. > >> By the way, why are you breaking out the elements of the > >> ARRAY_NOTATION_REF into a cilkplus_an_parts rather than using the > >> _REF directly? > > > > I use the different parts of array notations for error checking and to > > create the > outer for-loop. Also, to create the ARRAY_REF I need the induction variable. > > I understand the need for cilkplus_an_loop_parts, but cilkplus_an_parts seems > to contain exactly the same information as the ARRAY_NOTATION_REF. > > > Fixed! By the way, what is SFINAE? > > SFINAE stands for "substitution failure is not an error". During template > argument deduction, once we have a full set of template arguments we try to > substitute them into the template declaration. In that context, things that > would normally cause an error just fail and return error_mark_node silently. > > > diff --git a/gcc/cp/ChangeLog b/gcc/cp/ChangeLog index > > 55ed6a5..9d570b2 100644 Binary files a/gcc/cp/ChangeLog and > > b/gcc/cp/ChangeLog differ > > This patch still has ChangeLog entries. > This time, I ran the command you gave me. Please tell me how it looks. > > + new_var = get_temp_regvar (TREE_TYPE (retval_expr), > > +build_zero_cst (TREE_TYPE (retval_expr))); > >new_mod = expand_an_in_modify_expr (loc, new_var, NOP_EXPR, > > + TREE_OPERAND (retval_expr, 1), > > + tf_warning_or_error); > > Why not pass TREE_OPERAND (retval_expr, 1) as the init to get_temp_regvar? > new_var = TREE_OPERAND (retval_expr, 1) and if TREE_OPERAND (retval_expr, 1) has array notations then it wont get expanded correctly. Another solution is to replace get_tmp_regvar with get_temporary_var () + add_decl_expr (..). I have implemented this because it looks "more correct" > > - parser->colon_corrects_to_scope_p = > saved_colon_corrects_to_scope_p; > > if (!length_index || length_index == error_mark_node) > >
Re: [PATCH] Cilk Plus Array Notation for C++
On 06/25/2013 02:27 PM, Iyer, Balaji V wrote: This time, I ran the command you gave me. Please tell me how it looks. No ChangeLog this time, thanks. Another solution is to replace get_tmp_regvar with get_temporary_var () + add_decl_expr (..). I have implemented this because it looks "more correct" OK. I am setting the scope correction to false right before I look for length and restore it right after I parse the scope (i.e. outside the if-statement). I think this should fix the issue. OK. I think it would be better to convert start/stride to ptrdiff_t. I don't think I can do that. Stride can be negative and if I am not mistaken, ptrdiff_t is unsigned. You are mistaken. :) ptrdiff_t is the signed version of size_t. What I had in mind is that in the case of a normal array reference, cp_parser_array_notation will update index (which is passed by address) and return NULL_TREE, so that it gets back on the normal path. It doesn't look like you addressed this comment. + if (processing_template_decl) +{ + array_type = TREE_TYPE (array_value); + type = TREE_TYPE (array_type); +} We should be able to parse array notation in a template even when the array expression has unknown type. In a template, just parse and remember the raw expressions without worrying about diagnostics and conversions. Or this one. + /* If an array's index is an array notation, then its rank cannot be + greater than one. */ This one error is much easier to do it here than anywhere else. An array_ref could be a parameter inside a function, part of a modify expression, unary expression etc. If I move it to transformation stage, I have to do checks in all these places and there is a small chance some will slip through the cracks. This is almost a fool proof way of doing it. Such things have been done before. For example, Open MP does a return expression check in finish_return_stmt (even though this is a different issue we are talking about). What's the failure mode if one is missed? I would expect it to be pretty obvious. If it is the code lines that is an issue, then I am willing to enclose that in a function or #define. But I guess splitting it out into a separate function is OK. What remaining obstacles are there to sharing most of the expansion code between C and C++? That can be a separate patch, of course. Any thoughts? Jason
Re: [PATCH] Cilk Plus Array Notation for C++
On 06/25/13 13:42, Iyer, Balaji V wrote: What remaining obstacles are there to sharing most of the expansion code between C and C++? That can be a separate patch, of course. Jason ??
Re: [PATCH] Cilk Plus Array Notation for C++
On 06/26/2013 01:31 PM, Iyer, Balaji V wrote: Attached, please find a fixed patch and ChangeLog entries: This patch seems to be missing some hunks that are described in the ChangeLog and were present in the previous patch, such as * cp-array-notation.c (cp_length_mismatch_in_expr_p): Combined two if statements into one and compared integers using tree_int_cst_equal. Sorry about that, I accidentally missed this one. It is fixed now. I have also added the braced list capability into cp_array_notation. Hmm, I seem to have been unclear. I was expecting that the call to cp_parser_array_notation could come after the braced list case, so we don't need to duplicate the offsetof or braced list code inside cp_parser_array_notation. And then if you'd like we could check for ':' before the ']' and give a helpful diagnostic. + /* If we hare here, then there are 2 possibilities: "are" + if (processing_template_decl) +array_ntn_expr = build_min_nt_loc (loc, ARRAY_NOTATION_REF, array, + start_index, length, stride, NULL_TREE); If we know the type of the array, we should use it, rather than always leaving it null in a template. That is, if !dependent_type_p (type), we should give the ARRAY_NOTATION_REF a real type. + if (TREE_CODE (array_type) == RECORD_TYPE + || TREE_CODE (array_type) == POINTER_TYPE) { + error_at (loc, "start-index and length fields necessary for " + "using array notation in pointers or records"); In a template, array_type might be NULL_TREE; this diagnostic should move into build_array_notation_ref. + array_type_domain = TYPE_DOMAIN (array_type); + if (!array_type_domain) + { + error_at (loc, "start-index and length fields necessary for " + "using array notation with array of unknown bound"); + cp_parser_skip_to_end_of_statement (parser); + return error_mark_node; + } + start_index = TYPE_MINVAL (array_type_domain); + start_index = cp_fold_convert (ptrdiff_type_node, start_index); + length_index = size_binop + (PLUS_EXPR, TYPE_MAXVAL (array_type_domain), size_one_node); + length_index = cp_fold_convert (ptrdiff_type_node, length_index); + stride = build_int_cst (ptrdiff_type_node, 1); As should all this code. cp_parser_array_notation should only parse. + else + stride = build_one_cst (ptrdiff_type_node); I would move this into build_array_notation_ref as well. Jason
Re: [PATCH] Cilk Plus Array Notation for C++
On 06/27/2013 02:36 PM, Iyer, Balaji V wrote: I looked through the patch again. I was able to get rid of the first if-statement in cp_parser_postfix_open_square_expression function. I think now it looks exactly as you requested. Much better, thanks. + /* Sometimes, it type-casts certain functions to void. Unwrap it. */ + if (VOID_TYPE_P (TREE_TYPE (t)) && TREE_CODE (t) == CONVERT_EXPR) + t = TREE_OPERAND (t, 0); If something is typecast to void, we shouldn't need to keep it in a temporary; we should be able to just pass it directly to finish_expr_stmt and then replace other occurrences with void_zero_node. When are you encountering this? + if (TREE_CODE (type) == RECORD_TYPE || TREE_CODE (type) == POINTER_TYPE) + { + error_at (loc, "start-index and length fields necessary for " + "using array notation in pointers or records"); + return error_mark_node; + } I'd turn this around and for anything that isn't an array, say that the [:] notation can only be used with arrays. In particular it almost never makes sense to check for RECORD_TYPE specifically. if (TREE_CODE (type) == ARRAY_TYPE || TREE_CODE (type) == POINTER_TYPE) TREE_TYPE (array_ntn_expr) = TREE_TYPE (type); else TREE_TYPE (array_ntn_expr) = type; So the type of an ARRAY_NOTATION_EXPR where the "array" is a class object is that same class? That seems wrong. + bool saved_colon_corrects = parser->colon_corrects_to_scope_p; Let's declare this just before you change parser->colon_corrects_to_scope_p. + if (!*init_index || *init_index == error_mark_node) + cp_parser_skip_to_end_of_statement (parser); + length_index = cp_parser_expression (parser, false, NULL); It doesn't make sense to skip to the end of the statement and then try to keep parsing the stuff you just skipped over. + bool saved_colon_corrects = parser->colon_corrects_to_scope_p; + parser->colon_corrects_to_scope_p = false; + if (flag_enable_cilkplus + && cp_lexer_peek_token (parser->lexer)->type == CPP_COLON) + { + error_at (cp_lexer_peek_token (parser->lexer)->location, + "braced list index is not allowed with array " + "notation"); + cp_parser_skip_to_end_of_statement (parser); + return error_mark_node; + } + parser->colon_corrects_to_scope_p = saved_colon_corrects; You don't need to mess with colon_corrects_to_scope_p here; it doesn't affect the peek. Jason
Re: [PATCH] Cilk Plus Array Notation for C++
On 06/28/2013 12:54 AM, Iyer, Balaji V wrote: I agree with you and I have fixed it such that if TREE_TYPE is void then don't bother creating a new variable. This error happens in comma_exp.c testcase in Mac. For example, the following expression: array[:] = (atoi(argv[1]), (array2[0:10]+5)); In Mac, it is converting the above expression to array[:] = ((void) atoi (argv[1]), (array2[0:10]+5)) There is a function (replace_invariant_exprs) that will go through all the invariant expressions and replace it with a variable so that the function is only evaluated once. In this case, there is no reason to do so and just ignore it. We don't need to create a variable, but if the expression has side-effects I think we still want to move it out of the loop, right? The Cilk+ spec doesn't seem to specify whether subexpressions of rank 0 are evaluated once per iteration, or just once total. + /* Sometimes, when comma_expr has a function call in it, it will +typecast it to void. Find_inv_trees finds those nodes and so +if it void type, then don't bother creating a new var to hold +the return value. */ + if (VOID_TYPE_P (TREE_TYPE (t))) + new_var = t; + else + new_var = get_temp_regvar (TREE_TYPE (t), t); I was suggesting if (VOID_TYPE_P (TREE_TYPE (t))) { finish_expr_stmt (t); new_var = void_zero_node; } Jason
Re: [PATCH] Cilk Plus Array Notation for C++
On 06/28/2013 12:54 AM, Iyer, Balaji V wrote: /* If stride and start are of same type and the induction var is not, convert induction variable to stride's type. */ if (TREE_TYPE (start) == TREE_TYPE (stride) && TREE_TYPE (stride) != TREE_TYPE (var)) { st = start; str = stride; v = build_c_cast (loc, TREE_TYPE (str), var); } else if (TREE_TYPE (start) != TREE_TYPE (stride)) { /* 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, convert everything to integer. The reason why we pick an integer instead of something like size_t is because the stride and length can be + or -. */ st = build_c_cast (loc, ptrdiff_type_node, start); str = build_c_cast (loc, ptrdiff_type_node, stride); v = build_c_cast (loc, ptrdiff_type_node, var); } else { st = start; str = stride; v = var; } I still think that what you want is to unconditionally convert start and stride to ptrdiff_t, either here or in build_array_notation_ref. + tree ii_tree = array_exprs[ii][jj]; + (*node)[ii][jj].is_vector = true; + (*node)[ii][jj].value = ARRAY_NOTATION_ARRAY (ii_tree); + (*node)[ii][jj].start = ARRAY_NOTATION_START (ii_tree); + (*node)[ii][jj].length = + fold_build1 (CONVERT_EXPR, integer_type_node, + ARRAY_NOTATION_LENGTH (ii_tree)); + (*node)[ii][jj].stride = + fold_build1 (CONVERT_EXPR, integer_type_node, + ARRAY_NOTATION_STRIDE (ii_tree)); I still don't understand what the purpose of cilkplus_an_parts is; it seems to have exactly the same information as the ARRAY_NOTATION_REF, so you might as well keep the array of ARRAY_NOTATION_REF instead of copying it into an array of cilkplus_an_parts. + stride = RECUR (ARRAY_NOTATION_STRIDE (t)); + if (!cilkplus_an_triplet_types_ok_p (loc, start_index, length, stride, +TREE_TYPE (op1))) + RETURN (error_mark_node); You don't need to check the triplet types in tsubst, since you're checking them in build_array_notation_ref. Jason
Re: [PATCH] Cilk Plus Array Notation for C++
OK. Jason