RE: [patch] cilkplus array notation for C (clean, independent patchset, take 1)

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

Hello Joseph, Aldy et al.,
	Attached please find a patch that will fixed the problem below and another problem you mentioned in a previous email (I had used really_constant_p(..) and you mentioned that in C we need to check for INTEGER_CST).

Please let me know if I have missed anything else that you mentioned.

Thanks,

Balaji V. Iyer.

> -----Original Message-----
> From: Joseph Myers [mailto:joseph@codesourcery.com]
> Sent: Tuesday, March 26, 2013 1:05 PM
> To: Aldy Hernandez
> Cc: Iyer, Balaji V; gcc-patches@gcc.gnu.org
> Subject: Re: [patch] cilkplus array notation for C (clean, independent patchset,
> take 1)
> 
> On Mon, 25 Mar 2013, Aldy Hernandez wrote:
> 
> > > I always tend to check for a null pointer before I access the fields
> > > in the structure. In this case it is unnecessary. In some cases
> > > (e.g. find_rank) there is a good chance a null pointer will be
> > > passed into the function and we need to check that and reject those.
> >
> > I think what Joseph is suggesting is that if NULL is not valid, then
> > the caller should check this.  But if NULL is valid, then it should be
> > documented in the function comment at the top.
> 
> The caller should only check it if it's valid in the caller but not the callee.  If it's
> invalid in the caller as well, neither should check (except maybe in an assertion if
> felt appropriate in a particular case).

FIXED!

> 
> --
> Joseph S. Myers
> joseph@codesourcery.com

diff --git a/gcc/c-family/array-notation-common.c b/gcc/c-family/array-notation-common.c
index b775225..894c02a 100644
--- a/gcc/c-family/array-notation-common.c
+++ b/gcc/c-family/array-notation-common.c
@@ -152,7 +152,7 @@ extract_sec_implicit_index_arg (location_t location, tree fn)
   if (TREE_CODE (fn) == CALL_EXPR)
     {
       fn_arg = CALL_EXPR_ARG (fn, 0);
-      if (really_constant_p (fn_arg))
+      if (TREE_CODE (fn_arg) == INTEGER_CST)
 	return_int = int_cst_value (fn_arg);
       else
 	{
diff --git a/gcc/c/c-array-notation.c b/gcc/c/c-array-notation.c
index 9ee281e..dd0a1b0 100755
--- a/gcc/c/c-array-notation.c
+++ b/gcc/c/c-array-notation.c
@@ -52,9 +52,7 @@ struct inv_list
 /* Set *RANK of expression ARRAY, ignoring array notation specific built-in 
    functions if IGNORE_BUILTIN_FN is true.  The ORIG_EXPR is printed out if an
    error occured in the rank calculation.  The functions returns false if it
-   encounters an error in rank calculation.  If ARRAY can be NULL, since it is
-   recursively accessing all the fields in a subtree.  If so, then just return
-   true.
+   encounters an error in rank calculation.
 
    For example, an array notation of A[:][:] or B[0:10][0:5:2] or C[5][:][1:0]
    all have a rank of 2.  */
@@ -66,10 +64,8 @@ find_rank (location_t loc, tree orig_expr, tree array, bool ignore_builtin_fn,
   tree ii_tree;
   size_t ii = 0, current_rank = 0;
   an_reduce_type dummy_type = REDUCE_UNKNOWN;
-  
-  if (!array)
-    return true;
-  else if (TREE_CODE (array) == ARRAY_NOTATION_REF)
+ 
+  if (TREE_CODE (array) == ARRAY_NOTATION_REF)
     {
       for (ii_tree = array;
 	   ii_tree && TREE_CODE (ii_tree) == ARRAY_NOTATION_REF;
@@ -111,7 +107,8 @@ find_rank (location_t loc, tree orig_expr, tree array, bool ignore_builtin_fn,
 	}
       else 
 	for (ii = 0; ii < TREE_CODE_LENGTH (TREE_CODE (array)); ii++) 
-	  if (!find_rank (loc, orig_expr, TREE_OPERAND (array, ii),
+	  if (TREE_OPERAND (array, ii)
+	      && !find_rank (loc, orig_expr, TREE_OPERAND (array, ii),
 			  ignore_builtin_fn, rank))
 	    return false;
     }
@@ -133,21 +130,22 @@ extract_array_notation_exprs (tree node, bool ignore_builtin_fn,
   size_t ii = 0;
   an_reduce_type dummy_type = REDUCE_UNKNOWN;
   
-  if (!node)
-    return;
-  else if (TREE_CODE (node) == ARRAY_NOTATION_REF)
+  if (TREE_CODE (node) == ARRAY_NOTATION_REF)
     {
       vec_safe_push (*array_list, node);
       return;
     }
   else if (TREE_CODE (node) == TREE_LIST)
     {
-      extract_array_notation_exprs (TREE_PURPOSE (node), ignore_builtin_fn,
-				    array_list);
-      extract_array_notation_exprs (TREE_VALUE (node), ignore_builtin_fn,
-				    array_list);
-      extract_array_notation_exprs (TREE_CHAIN (node), ignore_builtin_fn,
-				    array_list);
+      if (TREE_PURPOSE (node))
+	extract_array_notation_exprs (TREE_PURPOSE (node), ignore_builtin_fn,
+				      array_list);
+      if (TREE_VALUE (node))
+	extract_array_notation_exprs (TREE_VALUE (node), ignore_builtin_fn,
+				      array_list);
+      if (TREE_CHAIN (node))
+	extract_array_notation_exprs (TREE_CHAIN (node), ignore_builtin_fn,
+				      array_list);
     }
   else if (TREE_CODE (node) == STATEMENT_LIST)
     {
@@ -181,8 +179,9 @@ extract_array_notation_exprs (tree node, bool ignore_builtin_fn,
     } 
   else 
     for (ii = 0; ii < TREE_CODE_LENGTH (TREE_CODE (node)); ii++) 
-      extract_array_notation_exprs (TREE_OPERAND (node, ii), 
-				    ignore_builtin_fn, array_list);
+      if (TREE_OPERAND (node, ii))
+	extract_array_notation_exprs (TREE_OPERAND (node, ii),
+				      ignore_builtin_fn, array_list);
   return;
 }
 
@@ -190,10 +189,7 @@ extract_array_notation_exprs (tree node, bool ignore_builtin_fn,
 /* Replaces all the occurances of array notations in *LIST with the appropriate
    one in ARRAY_OPERAND.  If IGNORE_BUILTIN_FN is set, then array notations
    inside array-notation specific builtin functions are ignored.  ORIG can be
-   anything from a collection of statement lists to a single variable.  Since
-   this function steps through all the subtrees, it is probable that *ORIG can
-   be a NULL_TREE.  If so, then the function just returns.
-*/
+   anything from a collection of statement lists to a single variable.  */
 
 static void
 replace_array_notations (tree *orig, bool ignore_builtin_fn,
@@ -204,7 +200,7 @@ replace_array_notations (tree *orig, bool ignore_builtin_fn,
   tree node = NULL_TREE, node_replacement = NULL_TREE;
   an_reduce_type dummy_type = REDUCE_UNKNOWN;
   
-  if (vec_safe_length (list) == 0 || !*orig)
+  if (vec_safe_length (list) == 0)
     return;
 
   if (TREE_CODE (*orig) == ARRAY_NOTATION_REF)
@@ -262,8 +258,9 @@ replace_array_notations (tree *orig, bool ignore_builtin_fn,
   else
     {
       for (ii = 0; ii < (size_t) TREE_CODE_LENGTH (TREE_CODE (*orig)); ii++) 
-	replace_array_notations (&TREE_OPERAND (*orig, ii), ignore_builtin_fn, 
-				 list, array_operand);
+	if (TREE_OPERAND (*orig, ii))
+	  replace_array_notations (&TREE_OPERAND (*orig, ii), ignore_builtin_fn,
+				   list, array_operand);
     }
   return;
 }
@@ -330,9 +327,7 @@ replace_inv_trees (tree *tp, int *walk_subtrees, void *data)
 
 /* Replaces all the scalar expressions in *NODE.  Returns a STATEMENT_LIST that
    holds the NODE along with variables that holds the results of the invariant
-   expressions.  Since this function steps through all the subtrees, it is
-   probable than *NODE or NODE can be NULL_TREE and NULL respectively.  If so,
-   then the function just returns NULL_TREE.  */
+   expressions.  */
 
 tree
 replace_invariant_exprs (tree *node)
@@ -342,9 +337,6 @@ replace_invariant_exprs (tree *node)
   tree t = NULL_TREE, new_var = NULL_TREE, new_node; 
   struct inv_list data;
 
-  if (!node || !*node)
-    return NULL_TREE;
-
   data.list_values = NULL;
   data.replacement = NULL;
   walk_tree (node, find_inv_trees, (void *)&data, NULL);
@@ -987,6 +979,8 @@ build_array_notation_expr (location_t location, tree lhs, tree lhs_origtype,
        build2 (PLUS_EXPR, TREE_TYPE (lhs_var[ii]), lhs_var[ii], 
 	       build_one_cst (TREE_TYPE (lhs_var[ii]))));
   
+  /* If array_expr_lhs is NULL, then we have function that returns void or
+     its return value is ignored.  */
   if (!array_expr_lhs)
     array_expr_lhs = lhs;
 
@@ -2372,9 +2366,6 @@ is_builtin_array_notation_fn (tree func_name, an_reduce_type *type)
 	function_name = IDENTIFIER_POINTER (DECL_NAME (func_name));
     }
   
-  if (!function_name)
-    return false;
-
   if (!strcmp (function_name, "__sec_reduce_add"))
     {
       *type = REDUCE_ADD;
@@ -2444,7 +2435,9 @@ is_builtin_array_notation_fn (tree func_name, an_reduce_type *type)
 }
 
 
-/* Returns true if EXPR (and its subtrees) contain ARRAY_NOTATION_EXPR node.  */
+/* Returns true if EXPR (and its subtrees) contain ARRAY_NOTATION_EXPR node.
+   If EXPR is NULL_TREE or does not contain any array notations, then the
+   function returns false.   */
 
 bool
 contains_array_notation_expr (tree expr)
@@ -2780,13 +2773,12 @@ fix_array_notation_call_expr (tree arg)
 /* Walks through tree node T and find all the call-statments that do not return
    anything and fix up any array notations they may carry.  The return value
    is the same type as T but with all array notations replaced with appropriate
-   STATEMENT_LISTS.  Since this function recursively steps through all the
-   subtrees, t can be a NULL_TREE.  If so, then the function just returns.  */
+   STATEMENT_LISTS.  */
 
 tree
 expand_array_notation_exprs (tree t)
 {
-  if (!t || !contains_array_notation_expr (t))
+  if (!contains_array_notation_expr (t))
     return t;
 
   switch (TREE_CODE (t))
@@ -2801,8 +2793,12 @@ expand_array_notation_exprs (tree t)
 	 subtrees.  */
       if (TREE_CODE (t) == COND_EXPR)
 	{
-	  COND_EXPR_THEN (t) = expand_array_notation_exprs (COND_EXPR_THEN (t));
-	  COND_EXPR_ELSE (t) = expand_array_notation_exprs (COND_EXPR_ELSE (t));
+	  if (COND_EXPR_THEN (t))
+	    COND_EXPR_THEN (t) =
+	      expand_array_notation_exprs (COND_EXPR_THEN (t));
+	  if (COND_EXPR_ELSE (t))
+	    COND_EXPR_ELSE (t) =
+	      expand_array_notation_exprs (COND_EXPR_ELSE (t));
 	}
       else
 	t = expand_array_notation_exprs (t);