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

["Iyer, Balaji V" <balaji dot v dot iyer at intel dot com>]

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