> -----Original Message----- > From: Jakub Jelinek [mailto:ja...@redhat.com] > Sent: Thursday, January 23, 2014 5:28 AM > To: Iyer, Balaji V > Cc: gcc-patches@gcc.gnu.org > Subject: Re: [PATCH] fix for PR 59825 > > 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)? >
Ok. I will commit the patch with the fix you suggested above. For your reference, I have attached a patch. Thanks, Balaji V. Iyer. > 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. > Ok. I will look into this.. > Jakub
diff --git a/gcc/c/ChangeLog b/gcc/c/ChangeLog index 8230c86..a158f11 100644 --- a/gcc/c/ChangeLog +++ b/gcc/c/ChangeLog @@ -1,5 +1,13 @@ 2014-01-23 Balaji V. Iyer <balaji.v.i...@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. + +2014-01-23 Balaji V. Iyer <balaji.v.i...@intel.com> + * c-parser.c (c_finish_omp_declare_simd): Made "cilk simd function" attribute an attribute without value. diff --git a/gcc/c/c-array-notation.c b/gcc/c/c-array-notation.c index 5526ee9..6a5631c --- a/gcc/c/c-array-notation.c +++ b/gcc/c/c-array-notation.c @@ -1218,22 +1218,21 @@ 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. */ -tree -expand_array_notation_exprs (tree t) +static tree +expand_array_notations (tree *tp, int *walk_subtrees, void *) { - if (!contains_array_notation_expr (t)) - return t; + if (!contains_array_notation_expr (*tp)) + { + *walk_subtrees = 0; + return NULL_TREE; + } + *walk_subtrees = 1; - switch (TREE_CODE (t)) + switch (TREE_CODE (*tp)) { - case BIND_EXPR: - t = expand_array_notation_exprs (BIND_EXPR_BODY (t)); - return t; case TRUTH_ORIF_EXPR: case TRUTH_ANDIF_EXPR: case TRUTH_OR_EXPR: @@ -1241,61 +1240,63 @@ expand_array_notation_exprs (tree t) case TRUTH_XOR_EXPR: case TRUTH_NOT_EXPR: case COND_EXPR: - t = fix_conditional_array_notations (t); - - /* After the expansion if they are still a COND_EXPR, we go into its - subtrees. */ - if (TREE_CODE (t) == COND_EXPR) - { - 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)); - } - return t; - case STATEMENT_LIST: - { - tree_stmt_iterator ii_tsi; - for (ii_tsi = tsi_start (t); !tsi_end_p (ii_tsi); tsi_next (&ii_tsi)) - *tsi_stmt_ptr (ii_tsi) = - expand_array_notation_exprs (*tsi_stmt_ptr (ii_tsi)); - } - return t; + *tp = fix_conditional_array_notations (*tp); + break; case MODIFY_EXPR: { - location_t loc = EXPR_HAS_LOCATION (t) ? EXPR_LOCATION (t) : + location_t loc = EXPR_HAS_LOCATION (*tp) ? EXPR_LOCATION (*tp) : UNKNOWN_LOCATION; - tree lhs = TREE_OPERAND (t, 0); - tree rhs = TREE_OPERAND (t, 1); + tree lhs = TREE_OPERAND (*tp, 0); + tree rhs = TREE_OPERAND (*tp, 1); location_t rhs_loc = EXPR_HAS_LOCATION (rhs) ? EXPR_LOCATION (rhs) : UNKNOWN_LOCATION; - t = build_array_notation_expr (loc, lhs, TREE_TYPE (lhs), NOP_EXPR, - rhs_loc, rhs, TREE_TYPE (rhs)); - return t; + *tp = build_array_notation_expr (loc, lhs, TREE_TYPE (lhs), NOP_EXPR, + rhs_loc, rhs, TREE_TYPE (rhs)); } + break; case CALL_EXPR: - t = fix_array_notation_call_expr (t); - return t; + *tp = fix_array_notation_call_expr (*tp); + break; case RETURN_EXPR: - if (contains_array_notation_expr (t)) - t = fix_return_expr (t); - return t; + *tp = fix_return_expr (*tp); + break; + case COMPOUND_EXPR: + if (TREE_CODE (TREE_OPERAND (*tp, 0)) == SAVE_EXPR) + { + /* In here we are calling expand_array_notations because + we need to be able to catch the return value and check if + it is an error_mark_node. */ + expand_array_notations (&TREE_OPERAND (*tp, 1), walk_subtrees, NULL); + + /* SAVE_EXPR cannot have an error_mark_node inside it. This check + will make sure that if there is an error in expanding of + array notations (e.g. rank mismatch) then replace the entire + SAVE_EXPR with an error_mark_node. */ + if (TREE_OPERAND (*tp, 1) == error_mark_node) + *tp = error_mark_node; + } + break; case ARRAY_NOTATION_REF: - /* IF we are here, then we are dealing with cases like this: + /* If we are here, then we are dealing with cases like this: A[:]; A[x:y:z]; A[x:y]; Replace those with just void zero node. */ - t = void_zero_node; + *tp = void_zero_node; default: - for (int ii = 0; ii < TREE_CODE_LENGTH (TREE_CODE (t)); ii++) - if (contains_array_notation_expr (TREE_OPERAND (t, ii))) - TREE_OPERAND (t, ii) = - expand_array_notation_exprs (TREE_OPERAND (t, ii)); - return t; + break; } + return NULL_TREE; +} + +/* Walks through tree node T and expands all array notations in its subtrees. + The return value is the same type as T but with all array notations + replaced with appropriate ARRAY_REFS with a loop around it. */ + +tree +expand_array_notation_exprs (tree t) +{ + walk_tree (&t, expand_array_notations, NULL, NULL); return t; }