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
