This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Cilk Plus Array Notation for C++
- From: Aldy Hernandez <aldyh at redhat dot com>
- To: "Iyer, Balaji V" <balaji dot v dot iyer at intel dot com>
- Cc: "gcc-patches at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>, "Jason Merrill (jason at redhat dot com)" <jason at redhat dot com>, "rth at redhat dot com" <rth at redhat dot com>
- Date: Wed, 12 Jun 2013 11:34:02 -0500
- Subject: Re: [PATCH] Cilk Plus Array Notation for C++
- References: <BF230D13CA30DD48930C31D4099330003A42D85F at FMSMSX101 dot amr dot corp dot intel dot com>
[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_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.
+ 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.
+ 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?
+ /* 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.
+ /* Disable correcting single colon correcting to scope. */
+ parser->colon_corrects_to_scope_p = false;
No need to document the obvious.
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".
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.
+ /* 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.
+ everything to integer. The reason why we pick integer
s/pick integer/pick an integer
Similarly in a few other places.
+/* 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)?
+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?
The fact that you are sometimes using "build_*" and sometimes "fix_*" to
denote expansion is confusing.
+/* 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
Thanks.
Aldy