[1/3,POPCOUNT] Handle COND_EXPR in expression_expensive_p

Message ID CAELXzTN9VCY=zsT7t5aex6GNEgXyG9TpGOPbNDgU4+pO5S6ZMA@mail.gmail.com
State New
Headers show
Series
  • [1/3,POPCOUNT] Handle COND_EXPR in expression_expensive_p
Related show

Commit Message

Kugan Vivekanandarajah June 22, 2018, 9:12 a.m.
[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.

Comments

Richard Biener June 25, 2018, 10:01 a.m. | #1
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.
Kugan Vivekanandarajah June 27, 2018, 4:59 a.m. | #2
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:
Richard Biener June 28, 2018, 11:26 a.m. | #3
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.
Kugan Vivekanandarajah July 5, 2018, 11:01 a.m. | #4
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:
Richard Biener July 5, 2018, 11:29 a.m. | #5
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.
Richard Biener July 5, 2018, 12:15 p.m. | #6
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.
Kugan Vivekanandarajah July 6, 2018, 9:45 a.m. | #7
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,
Richard Biener July 6, 2018, 10:17 a.m. | #8
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.
Kugan Vivekanandarajah July 9, 2018, 7:04 a.m. | #9
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)
Richard Biener July 9, 2018, 8:26 a.m. | #10
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.

Patch

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