| Message ID | CAELXzTN9VCY=zsT7t5aex6GNEgXyG9TpGOPbNDgU4+pO5S6ZMA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3,POPCOUNT] Handle COND_EXPR in expression_expensive_p | expand |
On Fri, Jun 22, 2018 at 11:13 AM Kugan Vivekanandarajah <kugan.vivekanandarajah@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. Richard. > gcc/ChangeLog: > > 2018-06-22 Kugan Vivekanandarajah <kuganv@linaro.org> > > * tree-scalar-evolution.c (expression_expensive_p): Handle COND_EXPR.
Hi Richard, Thanks for the review. On 25 June 2018 at 20:01, Richard Biener <richard.guenther@gmail.com> wrote: > On Fri, Jun 22, 2018 at 11:13 AM Kugan Vivekanandarajah > <kugan.vivekanandarajah@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? 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 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 <kuganv@linaro.org> >> >> * tree-scalar-evolution.c (expression_expensive_p): Handle COND_EXPR. From 8f59f05ad21ac218834547593a7f308b4f837b1e Mon Sep 17 00:00:00 2001 From: Kugan Vivekanandarajah <kugan.vivekanandarajah@linaro.org> Date: Fri, 22 Jun 2018 14:10:26 +1000 Subject: [PATCH 1/3] generate popcount when checked for zero Change-Id: Iff93a1bd58d12e5e6951bc15ebf5db2ec2c85c2e --- gcc/tree-scalar-evolution.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/gcc/tree-scalar-evolution.c b/gcc/tree-scalar-evolution.c index 4b0ec02..d992582 100644 --- a/gcc/tree-scalar-evolution.c +++ b/gcc/tree-scalar-evolution.c @@ -3508,6 +3508,13 @@ expression_expensive_p (tree expr) return false; } + 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))); + switch (TREE_CODE_CLASS (code)) { case tcc_binary:
On Wed, Jun 27, 2018 at 7:00 AM Kugan Vivekanandarajah <kugan.vivekanandarajah@linaro.org> wrote: > > Hi Richard, > > Thanks for the review. > > On 25 June 2018 at 20:01, Richard Biener <richard.guenther@gmail.com> wrote: > > On Fri, Jun 22, 2018 at 11:13 AM Kugan Vivekanandarajah > > <kugan.vivekanandarajah@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 <kuganv@linaro.org> > >> > >> * tree-scalar-evolution.c (expression_expensive_p): Handle COND_EXPR.
Hi Richard, Thanks for the review. On 28 June 2018 at 21:26, Richard Biener <richard.guenther@gmail.com> wrote: > On Wed, Jun 27, 2018 at 7:00 AM Kugan Vivekanandarajah > <kugan.vivekanandarajah@linaro.org> wrote: >> >> Hi Richard, >> >> Thanks for the review. >> >> On 25 June 2018 at 20:01, Richard Biener <richard.guenther@gmail.com> wrote: >> > On Fri, Jun 22, 2018 at 11:13 AM Kugan Vivekanandarajah >> > <kugan.vivekanandarajah@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 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. 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 <kuganv@linaro.org> >> >> >> >> * tree-scalar-evolution.c (expression_expensive_p): Handle COND_EXPR. From 9a9712c7ec4cc8dd3824ccb7ab441742b85bebbe Mon Sep 17 00:00:00 2001 From: Kugan Vivekanandarajah <kugan.vivekanandarajah@linaro.org> Date: Fri, 22 Jun 2018 14:10:26 +1000 Subject: [PATCH 1/3] generate popcount when checked for zero Change-Id: I951e6d487268b757cbdaa8dcf671ab1377490db6 --- gcc/gimplify.c | 2 +- gcc/gimplify.h | 1 + gcc/testsuite/gcc.dg/tree-ssa/pr64183.c | 2 +- gcc/testsuite/gcc.target/i386/pr85073.c | 2 +- gcc/tree-scalar-evolution.c | 12 ++++++++++++ 5 files changed, 16 insertions(+), 3 deletions(-) diff --git a/gcc/gimplify.c b/gcc/gimplify.c index 97543ed..4de98e5 100644 --- a/gcc/gimplify.c +++ b/gcc/gimplify.c @@ -3878,7 +3878,7 @@ gimplify_pure_cond_expr (tree *expr_p, gimple_seq *pre_p) EXPR is GENERIC, while tree_could_trap_p can be called only on GIMPLE. */ -static bool +bool generic_expr_could_trap_p (tree expr) { unsigned i, n; diff --git a/gcc/gimplify.h b/gcc/gimplify.h index dd0e4c0..62ca869 100644 --- a/gcc/gimplify.h +++ b/gcc/gimplify.h @@ -83,6 +83,7 @@ extern enum gimplify_status gimplify_arg (tree *, gimple_seq *, location_t, extern void gimplify_function_tree (tree); extern enum gimplify_status gimplify_va_arg_expr (tree *, gimple_seq *, gimple_seq *); +extern bool generic_expr_could_trap_p (tree expr); gimple *gimplify_assign (tree, tree, gimple_seq *); #endif /* GCC_GIMPLIFY_H */ diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr64183.c b/gcc/testsuite/gcc.dg/tree-ssa/pr64183.c index 7a854fc..50d0c5a 100644 --- a/gcc/testsuite/gcc.dg/tree-ssa/pr64183.c +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr64183.c @@ -1,5 +1,5 @@ /* { dg-do compile } */ -/* { dg-options "-O3 -fno-tree-vectorize -fdump-tree-cunroll-details" } */ +/* { dg-options "-O3 -fno-tree-vectorize -fdisable-tree-sccp -fdump-tree-cunroll-details" } */ int bits; unsigned int size; diff --git a/gcc/testsuite/gcc.target/i386/pr85073.c b/gcc/testsuite/gcc.target/i386/pr85073.c index 187102d..71a5d23 100644 --- a/gcc/testsuite/gcc.target/i386/pr85073.c +++ b/gcc/testsuite/gcc.target/i386/pr85073.c @@ -1,6 +1,6 @@ /* PR target/85073 */ /* { dg-do compile } */ -/* { dg-options "-O2 -mbmi" } */ +/* { dg-options "-O2 -mbmi -fdisable-tree-sccp" } */ int foo (unsigned x) diff --git a/gcc/tree-scalar-evolution.c b/gcc/tree-scalar-evolution.c index 4b0ec02..8e29005 100644 --- a/gcc/tree-scalar-evolution.c +++ b/gcc/tree-scalar-evolution.c @@ -3508,6 +3508,18 @@ expression_expensive_p (tree expr) return false; } + if (code == COND_EXPR) + return (expression_expensive_p (TREE_OPERAND (expr, 0)) + || (EXPR_P (TREE_OPERAND (expr, 1)) + && EXPR_P (TREE_OPERAND (expr, 2))) + /* If either branch has side effects or could trap. */ + || TREE_SIDE_EFFECTS (TREE_OPERAND (expr, 1)) + || generic_expr_could_trap_p (TREE_OPERAND (expr, 1)) + || TREE_SIDE_EFFECTS (TREE_OPERAND (expr, 0)) + || generic_expr_could_trap_p (TREE_OPERAND (expr, 0)) + || expression_expensive_p (TREE_OPERAND (expr, 1)) + || expression_expensive_p (TREE_OPERAND (expr, 2))); + switch (TREE_CODE_CLASS (code)) { case tcc_binary:
On Thu, Jul 5, 2018 at 1:02 PM Kugan Vivekanandarajah <kugan.vivekanandarajah@linaro.org> wrote: > > Hi Richard, > > Thanks for the review. > > On 28 June 2018 at 21:26, Richard Biener <richard.guenther@gmail.com> wrote: > > On Wed, Jun 27, 2018 at 7:00 AM Kugan Vivekanandarajah > > <kugan.vivekanandarajah@linaro.org> wrote: > >> > >> Hi Richard, > >> > >> Thanks for the review. > >> > >> On 25 June 2018 at 20:01, Richard Biener <richard.guenther@gmail.com> wrote: > >> > On Fri, Jun 22, 2018 at 11:13 AM Kugan Vivekanandarajah > >> > <kugan.vivekanandarajah@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 <kuganv@linaro.org> > >> >> > >> >> * tree-scalar-evolution.c (expression_expensive_p): Handle COND_EXPR.
On Thu, Jul 5, 2018 at 1:29 PM Richard Biener <richard.guenther@gmail.com> wrote: > > On Thu, Jul 5, 2018 at 1:02 PM Kugan Vivekanandarajah > <kugan.vivekanandarajah@linaro.org> wrote: > > > > Hi Richard, > > > > Thanks for the review. > > > > On 28 June 2018 at 21:26, Richard Biener <richard.guenther@gmail.com> wrote: > > > On Wed, Jun 27, 2018 at 7:00 AM Kugan Vivekanandarajah > > > <kugan.vivekanandarajah@linaro.org> wrote: > > >> > > >> Hi Richard, > > >> > > >> Thanks for the review. > > >> > > >> On 25 June 2018 at 20:01, Richard Biener <richard.guenther@gmail.com> wrote: > > >> > On Fri, Jun 22, 2018 at 11:13 AM Kugan Vivekanandarajah > > >> > <kugan.vivekanandarajah@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 <kuganv@linaro.org> > > >> >> > > >> >> * tree-scalar-evolution.c (expression_expensive_p): Handle COND_EXPR.
Hi Richard, > 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 Thanks. Is the attached patch OK? I am testing this on x86_64-linux-gnu and if there is no new regressions. Thanks, Kugan gcc/ChangeLog: 2018-07-06 Kugan Vivekanandarajah <kuganv@linaro.org> * tree-scalar-evolution.c (final_value_replacement_loop): Use rewrite_to_non_trapping_overflow instead of rewrite_to_defined_overflow. From 68a4f232f6cde68751f6785059121fe116363886 Mon Sep 17 00:00:00 2001 From: Kugan Vivekanandarajah <kugan.vivekanandarajah@linaro.org> Date: Fri, 6 Jul 2018 13:34:41 +1000 Subject: [PATCH] rewrite with rewrite_to_non_trapping_overflow Change-Id: Ica4407eab1c2b6f4190d8c0df6308154ffad2c1f --- gcc/tree-scalar-evolution.c | 20 +++----------------- 1 file changed, 3 insertions(+), 17 deletions(-) diff --git a/gcc/tree-scalar-evolution.c b/gcc/tree-scalar-evolution.c index 4b0ec02..3b4f0aa 100644 --- a/gcc/tree-scalar-evolution.c +++ b/gcc/tree-scalar-evolution.c @@ -267,6 +267,7 @@ along with GCC; see the file COPYING3. If not see #include "gimple-iterator.h" #include "gimplify-me.h" #include "tree-cfg.h" +#include "tree-eh.h" #include "tree-ssa-loop-ivopts.h" #include "tree-ssa-loop-manip.h" #include "tree-ssa-loop-niter.h" @@ -3616,24 +3617,9 @@ final_value_replacement_loop (struct loop *loop) && TYPE_OVERFLOW_UNDEFINED (TREE_TYPE (def))) { gimple_seq stmts; - gimple_stmt_iterator gsi2; + def = rewrite_to_non_trapping_overflow (def); def = force_gimple_operand (def, &stmts, true, NULL_TREE); - gsi2 = gsi_start (stmts); - while (!gsi_end_p (gsi2)) - { - gimple *stmt = gsi_stmt (gsi2); - gimple_stmt_iterator gsi3 = gsi2; - gsi_next (&gsi2); - gsi_remove (&gsi3, false); - if (is_gimple_assign (stmt) - && arith_code_with_undefined_signed_overflow - (gimple_assign_rhs_code (stmt))) - gsi_insert_seq_before (&gsi, - rewrite_to_defined_overflow (stmt), - GSI_SAME_STMT); - else - gsi_insert_before (&gsi, stmt, GSI_SAME_STMT); - } + gsi_insert_seq_before (&gsi, stmts, GSI_SAME_STMT); } else def = force_gimple_operand_gsi (&gsi, def, false, NULL_TREE,
On Fri, Jul 6, 2018 at 11:45 AM Kugan Vivekanandarajah <kugan.vivekanandarajah@linaro.org> wrote: > > Hi Richard, > > > 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 > Thanks. > > Is the attached patch OK? I am testing this on x86_64-linux-gnu and if > there is no new regressions. Please clean up the control flow to if (...) def = rewrite_to_non_trapping_overflow (def); def = force_gimple_operand_gsi (&gsi, def, false, NULL_TREE, true, GSI_SAME_STMT); OK with that change. Richard. > Thanks, > Kugan > > gcc/ChangeLog: > > 2018-07-06 Kugan Vivekanandarajah <kuganv@linaro.org> > > * tree-scalar-evolution.c (final_value_replacement_loop): Use > rewrite_to_non_trapping_overflow instead of rewrite_to_defined_overflow.
Hi Richard, Thanks for the review. On 6 July 2018 at 20:17, Richard Biener <richard.guenther@gmail.com> wrote: > On Fri, Jul 6, 2018 at 11:45 AM Kugan Vivekanandarajah > <kugan.vivekanandarajah@linaro.org> wrote: >> >> Hi Richard, >> >> > 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 >> Thanks. >> >> Is the attached patch OK? I am testing this on x86_64-linux-gnu and if >> there is no new regressions. > > Please clean up the control flow to > > if (...) > def = rewrite_to_non_trapping_overflow (def); > def = force_gimple_operand_gsi (&gsi, def, false, NULL_TREE, > true, GSI_SAME_STMT); I also had to add flag_trapv like we do in other places (for flag_wrapv) when calling rewrite_to_non_trapping_overflow. Attached patch bootstraps and there is no new regressions in x86-64-linux-gnu. Is this OK? Thanks, Kugan > > OK with that change. > Richard. > >> Thanks, >> Kugan >> >> gcc/ChangeLog: >> >> 2018-07-06 Kugan Vivekanandarajah <kuganv@linaro.org> >> >> * tree-scalar-evolution.c (final_value_replacement_loop): Use >> rewrite_to_non_trapping_overflow instead of rewrite_to_defined_overflow. From f3ecde5ff57d361e452965550aca94560629e784 Mon Sep 17 00:00:00 2001 From: Kugan Vivekanandarajah <kugan.vivekanandarajah@linaro.org> Date: Fri, 6 Jul 2018 13:34:41 +1000 Subject: [PATCH] rewrite with rewrite_to_non_trapping_overflow Change-Id: I18bb9713b4562cd3f3954c3997bb376969d8ce9b --- gcc/tree-scalar-evolution.c | 29 +++++++---------------------- 1 file changed, 7 insertions(+), 22 deletions(-) diff --git a/gcc/tree-scalar-evolution.c b/gcc/tree-scalar-evolution.c index 4b0ec02..4feb4b1 100644 --- a/gcc/tree-scalar-evolution.c +++ b/gcc/tree-scalar-evolution.c @@ -267,6 +267,7 @@ along with GCC; see the file COPYING3. If not see #include "gimple-iterator.h" #include "gimplify-me.h" #include "tree-cfg.h" +#include "tree-eh.h" #include "tree-ssa-loop-ivopts.h" #include "tree-ssa-loop-manip.h" #include "tree-ssa-loop-niter.h" @@ -3615,30 +3616,14 @@ final_value_replacement_loop (struct loop *loop) if (folded_casts && ANY_INTEGRAL_TYPE_P (TREE_TYPE (def)) && TYPE_OVERFLOW_UNDEFINED (TREE_TYPE (def))) { - gimple_seq stmts; - gimple_stmt_iterator gsi2; - def = force_gimple_operand (def, &stmts, true, NULL_TREE); - gsi2 = gsi_start (stmts); - while (!gsi_end_p (gsi2)) - { - gimple *stmt = gsi_stmt (gsi2); - gimple_stmt_iterator gsi3 = gsi2; - gsi_next (&gsi2); - gsi_remove (&gsi3, false); - if (is_gimple_assign (stmt) - && arith_code_with_undefined_signed_overflow - (gimple_assign_rhs_code (stmt))) - gsi_insert_seq_before (&gsi, - rewrite_to_defined_overflow (stmt), - GSI_SAME_STMT); - else - gsi_insert_before (&gsi, stmt, GSI_SAME_STMT); - } + bool saved_flag_trapv = flag_trapv; + flag_trapv = 1; + def = rewrite_to_non_trapping_overflow (def); + flag_trapv = saved_flag_trapv; } - else - def = force_gimple_operand_gsi (&gsi, def, false, NULL_TREE, - true, GSI_SAME_STMT); + def = force_gimple_operand_gsi (&gsi, def, false, NULL_TREE, + true, GSI_SAME_STMT); gassign *ass = gimple_build_assign (rslt, def); gsi_insert_before (&gsi, ass, GSI_SAME_STMT); if (dump_file)
On Mon, Jul 9, 2018 at 9:05 AM Kugan Vivekanandarajah <kugan.vivekanandarajah@linaro.org> wrote: > > Hi Richard, > > Thanks for the review. > > On 6 July 2018 at 20:17, Richard Biener <richard.guenther@gmail.com> wrote: > > On Fri, Jul 6, 2018 at 11:45 AM Kugan Vivekanandarajah > > <kugan.vivekanandarajah@linaro.org> wrote: > >> > >> Hi Richard, > >> > >> > 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 > >> Thanks. > >> > >> Is the attached patch OK? I am testing this on x86_64-linux-gnu and if > >> there is no new regressions. > > > > Please clean up the control flow to > > > > if (...) > > def = rewrite_to_non_trapping_overflow (def); > > def = force_gimple_operand_gsi (&gsi, def, false, NULL_TREE, > > true, GSI_SAME_STMT); > > I also had to add flag_trapv like we do in other places (for > flag_wrapv) when calling rewrite_to_non_trapping_overflow. Attached > patch bootstraps and there is no new regressions in x86-64-linux-gnu. > Is this OK? Aww, no. Sorry for misleading you - final value replacement - in addition to the trapping issue - needs to make sure to not introduce undefined overflow as well. So the rewrite_to_non_trapping_overflow should avoid the gimplification issue you ran into (and thus the extra predicate in expression_expensive) but you can't remove the rewrite_to_defined_overflow code. So, can you rework things again, doing the rewrtite_to_non_trapping_overflow but keep the rewrite_to_defined_overflow code as well? The you should be able to remove the generic_xpr_could_trap_p checks (and TREE_SIDE_EFFECTS). Thanks, Richard. > Thanks, > Kugan > > > > OK with that change. > > Richard. > > > >> Thanks, > >> Kugan > >> > >> gcc/ChangeLog: > >> > >> 2018-07-06 Kugan Vivekanandarajah <kuganv@linaro.org> > >> > >> * tree-scalar-evolution.c (final_value_replacement_loop): Use > >> rewrite_to_non_trapping_overflow instead of rewrite_to_defined_overflow.
From aa38b98dd97567c6032c261f19b3705abc2233b0 Mon Sep 17 00:00:00 2001 From: Kugan Vivekanandarajah <kugan.vivekanandarajah@linaro.org> Date: Fri, 22 Jun 2018 14:10:26 +1000 Subject: [PATCH 1/3] generate popcount when checked for zero Change-Id: I7255bf35e28222f7418852cb232246edf1fb5a39 --- gcc/tree-scalar-evolution.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/gcc/tree-scalar-evolution.c b/gcc/tree-scalar-evolution.c index 4b0ec02..db419a4 100644 --- a/gcc/tree-scalar-evolution.c +++ b/gcc/tree-scalar-evolution.c @@ -3508,6 +3508,11 @@ expression_expensive_p (tree expr) return false; } + if (code == COND_EXPR) + return (expression_expensive_p (TREE_OPERAND (expr, 0)) + || expression_expensive_p (TREE_OPERAND (expr, 1)) + || expression_expensive_p (TREE_OPERAND (expr, 2))); + switch (TREE_CODE_CLASS (code)) { case tcc_binary: -- 2.7.4
[PATCH 1/3][POPCOUNT] Handle COND_EXPR in expression_expensive_p gcc/ChangeLog: 2018-06-22 Kugan Vivekanandarajah <kuganv@linaro.org> * tree-scalar-evolution.c (expression_expensive_p): Handle COND_EXPR.