On Thu, Jul 5, 2018 at 1:02 PM Kugan Vivekanandarajah <kugan.vivekanandara...@linaro.org> wrote: > > Hi Richard, > > Thanks for the review. > > On 28 June 2018 at 21:26, Richard Biener <richard.guent...@gmail.com> wrote: > > 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. > > Is the attached patch which does this is OK?. I had to fix couple of > testcases because now the final value replacement removed the loop for > pr64183.c and pr85073.c is popcount pattern so I just disabled it so > that we can test what was tested earlier.
The patch is OK. > > > > 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. > Could you kindly refer me to Jakubs patch please. I couldn't find it quickly, asked Jakub now. Thanks, Richard. > > Thanks, > Kugan > > > > > > 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.
