On Thu, Jul 5, 2018 at 1:29 PM Richard Biener <richard.guent...@gmail.com> wrote: > > 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.
It was rewrite_to_non_trapping_overflow available in tree.h. Thus final value replacement could use that before gimplifying instead of using rewrite_to_defined_overflow Richard. > 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.
