On Wed, Jun 27, 2018 at 7:00 AM Kugan Vivekanandarajah
<kugan.vivekanandara...@linaro.org> wrote:
>
> Hi Richard,
>
> Thanks for the review.
>
> On 25 June 2018 at 20:01, Richard Biener <richard.guent...@gmail.com> wrote:
> > On Fri, Jun 22, 2018 at 11:13 AM Kugan Vivekanandarajah
> > <kugan.vivekanandara...@linaro.org> wrote:
> >>
> >> [PATCH 1/3][POPCOUNT] Handle COND_EXPR in expression_expensive_p
> >
> > This says that COND_EXPR itself isn't expensive. I think we should
> > constrain that a bit.
> > I think a good default would be to only allow a single COND_EXPR which
> > you can achieve
> > by adding a bool in_cond_expr_p = false argument to the function, pass
> > in_cond_expr_p
> > down and pass true down from the COND_EXPR handling itself.
> >
> > I'm not sure if we should require either COND_EXPR arm (operand 1 or
> > 2) to be constant
> > or !EXPR_P (then multiple COND_EXPRs might be OK).
> >
> > The main idea is to avoid evaluating many expressions but only
> > choosing one in the end.
> >
> > The simplest patch achieving that is sth like
> >
> > + if (code == COND_EXPR)
> > + return (expression_expensive_p (TREE_OPERAND (expr, 0))
> > || (EXPR_P (TREE_OPERAND (expr, 1)) && EXPR_P
> > (TREE_OPERAND (expr, 2)))
> > + || expression_expensive_p (TREE_OPERAND (expr, 1))
> > + || expression_expensive_p (TREE_OPERAND (expr, 2)));
> >
> > OK with that change.
>
> Is || (EXPR_P (TREE_OPERAND (expr, 1)) || EXPR_P (TREE_OPERAND (expr,
> 2))) slightly better ?
> Attaching with the change. Is this OK?
Well, it won't allow x != 0 ? popcount (x) : 1 because popcount(x) is CALL_EXPR.
>
>
> Because, for pr81661.c, we now allow as not expensive
> <plus_expr 0x7ffff6a87b40
> type <integer_type 0x7ffff69455e8 int public SI
> size <integer_cst 0x7ffff692df30 constant 32>
> unit-size <integer_cst 0x7ffff692df48 constant 4>
> align:32 warn_if_not_align:0 symtab:0 alias-set 1
> canonical-type 0x7ffff69455e8 precision:32 min <integer_cst
> 0x7ffff692dee8 -2147483648> max <integer_cst 0x7ffff692df00
> 2147483647>
> pointer_to_this <pointer_type 0x7ffff694d9d8>>
>
> arg:0 <plus_expr 0x7ffff6a87b68 type <integer_type 0x7ffff69455e8 int>
>
> arg:0 <ssa_name 0x7ffff6937bd0 type <integer_type 0x7ffff69455e8 int>
> visited
> def_stmt a.1_10 = a;
> version:10>
> arg:1 <integer_cst 0x7ffff694a0d8 constant -1>>
> arg:1 <cond_expr 0x7ffff6a89000 type <integer_type 0x7ffff69455e8 int>
>
> arg:0 <ge_expr 0x7ffff6a87b90 type <boolean_type 0x7ffff6945b28 _Bool>
>
> arg:0 <plus_expr 0x7ffff6a87bb8 type <integer_type
> 0x7ffff69455e8 int>
>
> arg:0 <plus_expr 0x7ffff6a87be0 type <integer_type
> 0x7ffff69455e8 int>
> arg:0 <ssa_name 0x7ffff6937bd0> arg:1 <integer_cst
> 0x7ffff694a0d8 -1>>
> arg:1 <ssa_name 0x7ffff6937c18 type <integer_type
> 0x7ffff69455e8 int>
> visited
> def_stmt c.2_11 = c;
> version:11>>
> arg:1 <ssa_name 0x7ffff6937ca8 type <integer_type
> 0x7ffff69455e8 int>
> visited
> def_stmt b.3_13 = b;
> version:13>>
> arg:1 <negate_expr 0x7ffff6a88560 type <integer_type 0x7ffff69455e8
> int>
>
> arg:0 <nop_expr 0x7ffff6a88580 type <integer_type
> 0x7ffff69455e8 int>
>
> arg:0 <minus_expr 0x7ffff6a87c08 type <integer_type
> 0x7ffff6a55b28>
>
> arg:0 <nop_expr 0x7ffff6a885a0 type <integer_type
> 0x7ffff6a55b28>
>
> arg:0 <plus_expr 0x7ffff6a87c30 type
> <integer_type 0x7ffff69455e8 int>
> arg:0 <plus_expr 0x7ffff6a87c58> arg:1
> <ssa_name 0x7ffff6937c18>>>
> arg:1 <nop_expr 0x7ffff6a885c0 type <integer_type
> 0x7ffff6a55b28>
> arg:0 <ssa_name 0x7ffff6937ca8>>>>>
> arg:2 <integer_cst 0x7ffff694a090 constant 0>>>
>
> Which also leads to an ICE in gimplify_modify_expr. I think this is a
> latent issue and I am trying to find the source
Well, I think that's because some COND_EXPRs only gimplify to
conditional code. See gimplify_cond_expr:
if (gimplify_ctxp->allow_rhs_cond_expr
/* If either branch has side effects or could trap, it can't be
evaluated unconditionally. */
&& !TREE_SIDE_EFFECTS (then_)
&& !generic_expr_could_trap_p (then_)
&& !TREE_SIDE_EFFECTS (else_)
&& !generic_expr_could_trap_p (else_))
return gimplify_pure_cond_expr (expr_p, pre_p);
so we probably have to treat TREE_SIDE_EFFECTS / generic_expr_could_trap_p as
"expensive" as well for the purpose of final value replacement unless we are
going to support a code-generation way different from gimplification.
The testcase you cite uses -ftrapv which is why we run into this issue. Note
that final value replacement deals with this by rewriting the expression to
unsigned but it does so only after gimplification. IIRC Jakub recently
added a helper to rewrite GENERIC to unsigned so that might be useful
in this context.
Richard.
> the expr in gimple_modify_expr is
> <modify_expr 0x7ffff6a87cd0
> type <integer_type 0x7ffff69455e8 int public SI
> size <integer_cst 0x7ffff692df30 constant 32>
> unit-size <integer_cst 0x7ffff692df48 constant 4>
> align:32 warn_if_not_align:0 symtab:0 alias-set 1
> canonical-type 0x7ffff69455e8 precision:32 min <integer_cst
> 0x7ffff692dee8 -2147483648> max <integer_cst 0x7ffff692df00
> 2147483647>
> pointer_to_this <pointer_type 0x7ffff694d9d8>>
> side-effects
> arg:0 <var_decl 0x7ffff6a802d0 iftmp.5 type <integer_type
> 0x7ffff69455e8 int>
> used ignored SI (null):0:0 size <integer_cst 0x7ffff692df30
> 32> unit-size <integer_cst 0x7ffff692df48 4>
> align:32 warn_if_not_align:0 context <function_decl 0x7ffff6a57700
> foo>>
> arg:1 <negate_expr 0x7ffff6a88560 type <integer_type 0x7ffff69455e8 int>
>
> arg:0 <nop_expr 0x7ffff6a88580 type <integer_type 0x7ffff69455e8 int>
>
> arg:0 <minus_expr 0x7ffff6a87c08 type <integer_type
> 0x7ffff6a55b28>
>
> arg:0 <nop_expr 0x7ffff6a885a0 type <integer_type
> 0x7ffff6a55b28>
>
> arg:0 <plus_expr 0x7ffff6a87c30 type <integer_type
> 0x7ffff69455e8 int>
>
> arg:0 <plus_expr 0x7ffff6a87c58 type
> <integer_type 0x7ffff69455e8 int>
> arg:0 <ssa_name 0x7ffff6937bd0> arg:1
> <integer_cst 0x7ffff694a0d8 -1>>
> arg:1 <ssa_name 0x7ffff6937c18 type
> <integer_type 0x7ffff69455e8 int>
> visited
> def_stmt c.2_11 = c;
> version:11>>>
> arg:1 <nop_expr 0x7ffff6a885c0 type <integer_type
> 0x7ffff6a55b28>
>
> arg:0 <ssa_name 0x7ffff6937ca8 type <integer_type
> 0x7ffff69455e8 int>
> visited
> def_stmt b.3_13 = b;
> version:13>>>>>>
>
> and the *to_p is not SSA_NAME and VAR_DECL.
>
> Thanks,
> Kugan
>
>
>
> >
> > Richard.
> >
> >> gcc/ChangeLog:
> >>
> >> 2018-06-22 Kugan Vivekanandarajah <kug...@linaro.org>
> >>
> >> * tree-scalar-evolution.c (expression_expensive_p): Handle COND_EXPR.
