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

2013-06-28 Thread Jason Merrill 

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++

2013-06-28 Thread Jason Merrill 

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++

2013-06-28 Thread Jason Merrill 

OK.

Jason

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

2013-06-27 Thread Jason Merrill 

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++

2013-06-26 Thread Aldy Hernandez 

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++

2013-06-26 Thread Jason Merrill 

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++

2013-06-25 Thread Jason Merrill 

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_WIDE_INT r_length = int_cst_value

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

2013-06-25 Thread Iyer, Balaji V 


 -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)
  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.

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

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

2013-06-25 Thread Jason Merrill 

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++

2013-06-24 Thread Jason Merrill 

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++

2013-06-22 Thread Jason Merrill 
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 simpler.


Actually, to reduce

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

2013-06-21 Thread Richard Henderson 
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++

2013-06-21 Thread Iyer, Balaji V 


 -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++

2013-06-21 Thread Richard Henderson 
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++

2013-06-20 Thread Richard Henderson 
 +/* 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, vecvectree 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 vectree  *value, vecvectree  *start,
 +   vec vectree  *length, vecvectree  *stride,
 +   vecvecbool  *is_vector, vecvecbool  *count_down,
 +   vectree *incr, vectree *cmp, vectree *ind_init,
 +   vectree *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++

2013-06-18 Thread Aldy Hernandez 

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++

2013-06-17 Thread Iyer, Balaji V 


 -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++

2013-06-17 Thread Richard Henderson 
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++

2013-06-13 Thread Aldy Hernandez 



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++

2013-06-13 Thread Richard Henderson 
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++

2013-06-12 Thread Aldy Hernandez 

[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

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

2013-06-12 Thread Iyer, Balaji V 
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_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

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

2013-06-12 Thread Aldy Hernandez 



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++

2013-06-12 Thread Iyer, Balaji V 


 -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.