Re: [PATCH] Cilk Plus Array Notation for C++

[Aldy Hernandez <aldyh at redhat 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