On October 22, 2014 5:08:50 PM CEST, Jakub Jelinek <ja...@redhat.com> wrote: >On Wed, Oct 22, 2014 at 04:20:09PM +0200, Richard Biener wrote: >> 2014-10-22 Richard Biener <rguent...@suse.de> >> >> * genmatch.c (count_captures): New function. >> (dt_simplify::gen): Handle preserving side-effects for >> GENERIC code generation. >> (decision_tree::gen_generic): Do not reject operands >> with TREE_SIDE_EFFECTS. >> >> Index: gcc/genmatch.c >> =================================================================== >> --- gcc/genmatch.c (revision 216549) >> +++ gcc/genmatch.c (working copy) >> @@ -1861,6 +1861,46 @@ dt_operand::gen (FILE *f, bool gimple) >> fprintf (f, "}\n"); >> } >> >> +/* Count how many times captures are substituted in O and put the >> + result in the array of counters CPT. Return false if the >> + counting isn't precise. */ >> + >> +static bool >> +count_captures (operand *o, int *cpt) >> +{ >> + if (capture *c = dyn_cast <capture *> (o)) >> + { >> + /* Give up for non-leafs (CSEs). */ >> + if (c->what) >> + return false; >> + cpt[c->where]++; >> + } >> + else if (expr *e = dyn_cast <expr *> (o)) >> + { >> + /* Give up for conditionally executed operands. */ >> + if (*e->operation == COND_EXPR >> + || *e->operation == TRUTH_ANDIF_EXPR >> + || *e->operation == TRUTH_ORIF_EXPR) >> + return false; > >As discussed on IRC, the problem is only captures inside of 2nd and 3rd >operand of COND_EXPR or 2nd operand of TRUTH_*IF_EXPR if the >first operand of those isn't constant (if it is constant, then >depending >on the zero/nonzero value it should be either counted or ignored). >Captures in first operands of those are not a problem.
Yeah - this was by no means supposed to be an optimal solution. But I just noticed we can fail to identify leafs as not all leafs need to be captured. Also I forgot to check for captures in with-exprs which can be used to transfer parts of the expression as well. Meanwhile chasing another latent bug... Richard. >> @@ -1983,10 +2042,21 @@ dt_simplify::gen (FILE *f, bool gimple) >> } >> else /* GENERIC */ >> { >> + bool is_predicate = false; >> if (result->type == operand::OP_EXPR) >> { >> expr *e = as_a <expr *> (result); >> - bool is_predicate = is_a <predicate_id *> (e->operation); >> + is_predicate = is_a <predicate_id *> (e->operation); >> + /* Search for captures used multiple times in the result >expression >> + and dependent on TREE_SIDE_EFFECTS emit a SAVE_EXPR. */ >> + if (!is_predicate && cpt[0] != -1) >> + for (int i = 0; i < s->capture_max + 1; ++i) >> + { >> + if (cpt[i] > 1) >> + fprintf (f, " if (TREE_SIDE_EFFECTS (captures[%d]))\n" >> + " captures[%d] = save_expr (captures[%d]);\n", >> + i, i, i); >> + } > >For SAVE_EXPRs this will not do anything, which is right. > >> + if (!is_predicate) >> + { >> + /* Search for captures not used in the result expression and >dependent >> + on TREE_SIDE_EFFECTS emit omit_one_operand. */ >> + if (cpt[0] != -1) >> + for (int i = 0; i < s->capture_max + 1; ++i) >> + { >> + if (cpt[i] == 0) >> + fprintf (f, " if (TREE_SIDE_EFFECTS (captures[%d]))\n" >> + " res = build2_loc (loc, COMPOUND_EXPR, " >> + "TREE_TYPE (captures[%d]), " >> + "fold_ignored_result (captures[%d]), res);\n", >> + i, i, i); >> + } >> + fprintf (f, " return res;\n"); >> + } > >For this case if captures[%d] happens to be a SAVE_EXPR, you >could perhaps avoid the COMPOUND_EXPR addition if the SAVE_EXPR is >already >present somewhere else in the res (in unconditional context). Not sure >if it is worth the check (pretty much it would want a function which >would >only add the COMPOUND_EXPR if the capture is not SAVE_EXPR or not found >in >the expression). > >Also, don't you want TREE_NO_WARNING on the COMPOUND_EXPR to avoid >-Wunused-value warnings (at least temporarily until C++ FE is fixed not >to >fold everything early)? > >And, perhaps it is better to queue all side-effects in lhs of toplevel >COMPOUND_EXPR, rather than having a chain of COMPOUND_EXPRs on the rhs? >I.e. first queue all side-effects into some temporary and if that >temporary >is non-NULL, just add a single COMPOUND_EXPR with res as rhs and that >temporary as lhs. > >Do we want to special case COMPOUND_EXPRs in further optimizations, or >will >it be handled automatically (I mean, if trying to simplify >(something, expr1) op expr2 >where expr1 op expr2 simplifies into something into >(something, result_of_simplification (expr1 op expr2)). > > Jakub