fix for PR 59825

> -----Original Message-----
> From: Jakub Jelinek [mailto:jakub@redhat.com]
> Sent: Wednesday, January 15, 2014 5:55 PM
> To: Iyer, Balaji V
> Cc: gcc-patches@gcc.gnu.org
> Subject: Re: [PATCH] fix for PR 59825
> 
> On Wed, Jan 15, 2014 at 10:37:04PM +0000, Iyer, Balaji V wrote:
> > Hello Everyone,
> 
> > Attached, please find a patch that will fix PR 59825.  The main issue
> > was array notations occurring in COMPOUND_EXPR.  This patch should fix
> > that and fix the rank_mismatch2.c test-case ICE.
> 
> > --- a/gcc/c/c-array-notation.c
> > +++ b/gcc/c/c-array-notation.c
> > @@ -1289,6 +1289,15 @@ expand_array_notation_exprs (tree t)
> >  	 A[x:y];
> >  	 Replace those with just void zero node.  */
> >        t = void_zero_node;
> > +      return t;
> > +    case COMPOUND_EXPR:
> > +      if (contains_array_notation_expr (t))
> > +	if (TREE_CODE (TREE_OPERAND (t, 0)) == SAVE_EXPR)
> > +	  {
> > +	    t = expand_array_notation_exprs (TREE_OPERAND (t, 1));
> > +	    return t;
> > +	  }
> > +         /* Else fall through.  */
> >      default:
> >        for (int ii = 0; ii < TREE_CODE_LENGTH (TREE_CODE (t)); ii++)
> >  	if (contains_array_notation_expr (TREE_OPERAND (t, ii)))
> 
> Why doesn't the default case handle it?  Furthermore, you are removing the
> COMPOUND_EXPR and the SAVE_EXPR from the first operand of the
> COMPOUND_EXPR, that reverts the effects of the fix if there are array
> notations anywhere.
> 
> And last comment to the expand_array_notation_exprs, especially the C++
> one, wouldn't it be better to rewrite them as walk_tree/cp_walk_tree
> callbacks, so that it really handles all expressions, not just a small subset of
> them?
> E.g. in C++ you just don't look at all about OMP_PARALLEL etc., so I'd expect
> you ICE if array notation is found inside of #pragma omp parallel body.

Hi Jakub,
	Attached, please find a fixed patch where I rewrote the expand_array_notation_exprs using walk_trees. In this implementation, I am also not reverting the effects of compound or save exprs.  Is this OK for trunk?

Here are the Changelog entries:
+2014-01-20  Balaji V. Iyer  <balaji.v.iyer@intel.com>
+
+       PR c/59825
+       * c-array-notation.c (expand_array_notation_exprs): Rewrote this
+       function to use walk_tree and moved a lot of its functionality to
+       expand_array_notations.
+       (expand_array_notations): New function.
+

Thanks,

Balaji V. Iyer.

> 
> 	Jakub

On Mon, Jan 20, 2014 at 10:40:31PM +0000, Iyer, Balaji V wrote:
> --- a/gcc/c/c-array-notation.c
> +++ b/gcc/c/c-array-notation.c
> @@ -1218,22 +1218,22 @@ fix_return_expr (tree expr)
>    return new_mod_list;
>  }
>  
> -/* Walks through tree node T and find all the call-statements 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.  */
> +/* Callback for walk_tree.  Expands all array notations in *TP.  *WALK_SUBTREES
> +   is set to 1 unless *TP contains no array notation expressions.  Parameter 
> +   D is unused.  */
>  
> -tree
> -expand_array_notation_exprs (tree t)
> +static tree
> +expand_array_notations (tree *tp, int *walk_subtrees, void *d ATTRIBUTE_UNUSED)

As we are now using C++, just use
expand_array_notations (tree *tp, int *walk_subtrees, void *)
and remove the comment about unused parameter D.

>  {
> -  if (!contains_array_notation_expr (t))
> -    return t;
> +  if (!contains_array_notation_expr (*tp))

Why do you want to walk the whole subtree once more at every level?
That has bad time complexity for very deep trees.
Can't you just do that in expand_array_notations_exprs once?

	Jakub

On Thu, Jan 23, 2014 at 10:27:58AM +0100, Jakub Jelinek wrote:
> On Mon, Jan 20, 2014 at 10:40:31PM +0000, Iyer, Balaji V wrote:
> > --- a/gcc/c/c-array-notation.c
> > +++ b/gcc/c/c-array-notation.c
> > @@ -1218,22 +1218,22 @@ fix_return_expr (tree expr)
> >    return new_mod_list;
> >  }
> >  
> > -/* Walks through tree node T and find all the call-statements 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.  */
> > +/* Callback for walk_tree.  Expands all array notations in *TP.  *WALK_SUBTREES
> > +   is set to 1 unless *TP contains no array notation expressions.  Parameter 
> > +   D is unused.  */
> >  
> > -tree
> > -expand_array_notation_exprs (tree t)
> > +static tree
> > +expand_array_notations (tree *tp, int *walk_subtrees, void *d ATTRIBUTE_UNUSED)
> 
> As we are now using C++, just use
> expand_array_notations (tree *tp, int *walk_subtrees, void *)
> and remove the comment about unused parameter D.
> 
> >  {
> > -  if (!contains_array_notation_expr (t))
> > -    return t;
> > +  if (!contains_array_notation_expr (*tp))
> 
> Why do you want to walk the whole subtree once more at every level?
> That has bad time complexity for very deep trees.
> Can't you just do that in expand_array_notations_exprs once?

Though, as this is not a regression with this patch and the patch fixes
a bug in the testsuite, perhaps you can do that incrementally.

So, can you please change the comment and expand_array_notations
prototype, then commit (patch preapproved)?

Then please add to todo list that walking all subtrees twice at every level
is something you should avoid.  Perhaps for the trees you don't explictly
handle you could gather in *(bool *)d whether it contained any array
notations, and trees you actually handle could first walk the subtrees
manually to gather the flags and transform what needs to be transformed in
there, and then just based on the bool flag decide if it should do anything
and what.  Also, how is e.g. array notation supposed to be handled in case
of nested MODIFY_EXPRs where the nested MODIFY_EXPRs contain array
notations?  I mean something like:
a[0:10] = (b[0:10] ? (c[0:10] = d[0:10] + (e[0:10] = f[0:10] ? (g[0:10] * 3 : g[0:10] + 12))) : 5);
etc.?
I'd say contains_array_notation_expr should also be rewritten to use
walk_tree, so should be the C++ AN expansion etc.  Just you'll need to
use cp_walk_tree for C++ probably and thus the c-family/ code should just
contain callbacks that you can use for the walking, not the actual calls
to walk_tree/cp_walk_tree.

	Jakub