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

Cilk Plus Array Notation for C++

Commit Message

Comments

Patch