Re: [PATCH 1/3][POPCOUNT] Handle COND_EXPR in expression_expensive_p
On Mon, Jul 9, 2018 at 9:05 AM Kugan Vivekanandarajah wrote: > > Hi Richard, > > Thanks for the review. > > On 6 July 2018 at 20:17, Richard Biener wrote: > > On Fri, Jul 6, 2018 at 11:45 AM Kugan Vivekanandarajah > > 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 > >> > >> * tree-scalar-evolution.c (final_value_replacement_loop): Use > >> rewrite_to_non_trapping_overflow instead of > >> rewrite_to_defined_overflow.
Re: [PATCH 1/3][POPCOUNT] Handle COND_EXPR in expression_expensive_p
Hi Richard,
Thanks for the review.
On 6 July 2018 at 20:17, Richard Biener wrote:
> On Fri, Jul 6, 2018 at 11:45 AM Kugan Vivekanandarajah
> 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
>>
>> * 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
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)
--
2.7.4
Re: [PATCH 1/3][POPCOUNT] Handle COND_EXPR in expression_expensive_p
On Fri, Jul 6, 2018 at 11:45 AM Kugan Vivekanandarajah 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 > > * tree-scalar-evolution.c (final_value_replacement_loop): Use > rewrite_to_non_trapping_overflow instead of rewrite_to_defined_overflow.
Re: [PATCH 1/3][POPCOUNT] Handle COND_EXPR in expression_expensive_p
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
* 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
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,
--
2.7.4
Re: [PATCH 1/3][POPCOUNT] Handle COND_EXPR in expression_expensive_p
On Thu, Jul 5, 2018 at 1:29 PM Richard Biener wrote: > > On Thu, Jul 5, 2018 at 1:02 PM Kugan Vivekanandarajah > wrote: > > > > Hi Richard, > > > > Thanks for the review. > > > > On 28 June 2018 at 21:26, Richard Biener wrote: > > > On Wed, Jun 27, 2018 at 7:00 AM Kugan Vivekanandarajah > > > wrote: > > >> > > >> Hi Richard, > > >> > > >> Thanks for the review. > > >> > > >> On 25 June 2018 at 20:01, Richard Biener > > >> wrote: > > >> > On Fri, Jun 22, 2018 at 11:13 AM Kugan Vivekanandarajah > > >> > 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 > > >> > >> type > >> size > > >> unit-size > > >> align:32 warn_if_not_align:0 symtab:0 alias-set 1 > > >> canonical-type 0x769455e8 precision:32 min > >> 0x7692dee8 -2147483648> max > >> 2147483647> > > >> pointer_to_this > > > >> > > >> arg:0 > >> int> > > >> > > >> arg:0 > >> int> > > >> visited > > >> def_stmt a.1_10 = a; > > >> version:10> > > >> arg:1 > > > >> arg:1 > >> int> > > >> > > >> arg:0 > >> _Bool> > > >> > > >> arg:0 > >> 0x769455e8 int> > > >> > > >> arg:0 > >> 0x769455e8 int> > > >> arg:0 arg:1 > >> 0x7694a0d8 -1>> > > >> arg:1 > >> 0x769455e8 int> > > >> visited > > >> def_stmt c.2_11 = c; > > >> version:11>> > > >> arg:1 > >> 0x769455e8 int> > > >> visited > > >> def_stmt b.3_13 = b; > > >> version:13>> > > >> arg:1 > >> 0x769455e8 int> > > >> > > >> arg:0 > >> 0x769455e8 int> > > >> > > >> arg:0 > >> 0x76a55b28> > > >> > > >> arg:0 > >> 0x76a55b28> > > >> > > >> arg:0 > >> > > >> arg:0 arg:1 > > >> >> > > >>
Re: [PATCH 1/3][POPCOUNT] Handle COND_EXPR in expression_expensive_p
On Thu, Jul 5, 2018 at 1:02 PM Kugan Vivekanandarajah wrote: > > Hi Richard, > > Thanks for the review. > > On 28 June 2018 at 21:26, Richard Biener wrote: > > On Wed, Jun 27, 2018 at 7:00 AM Kugan Vivekanandarajah > > wrote: > >> > >> Hi Richard, > >> > >> Thanks for the review. > >> > >> On 25 June 2018 at 20:01, Richard Biener > >> wrote: > >> > On Fri, Jun 22, 2018 at 11:13 AM Kugan Vivekanandarajah > >> > 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 > >> >> type >> size > >> unit-size > >> align:32 warn_if_not_align:0 symtab:0 alias-set 1 > >> canonical-type 0x769455e8 precision:32 min >> 0x7692dee8 -2147483648> max >> 2147483647> > >> pointer_to_this > > >> > >> arg:0 > >> > >> arg:0 >> int> > >> visited > >> def_stmt a.1_10 = a; > >> version:10> > >> arg:1 > > >> arg:1 > >> > >> arg:0 >> _Bool> > >> > >> arg:0 >> 0x769455e8 int> > >> > >> arg:0 >> 0x769455e8 int> > >> arg:0 arg:1 >> 0x7694a0d8 -1>> > >> arg:1 >> 0x769455e8 int> > >> visited > >> def_stmt c.2_11 = c; > >> version:11>> > >> arg:1 >> 0x769455e8 int> > >> visited > >> def_stmt b.3_13 = b; > >> version:13>> > >> arg:1 >> 0x769455e8 int> > >> > >> arg:0 >> 0x769455e8 int> > >> > >> arg:0 >> 0x76a55b28> > >> > >> arg:0 >> 0x76a55b28> > >> > >> arg:0 >> > >> arg:0 arg:1 > >> >> > >> arg:1 >> 0x76a55b28> > >> arg:0 >>>> > >> arg:2 >> > >> > >> 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. */ &
Re: [PATCH 1/3][POPCOUNT] Handle COND_EXPR in expression_expensive_p
Hi Richard, Thanks for the review. On 28 June 2018 at 21:26, Richard Biener wrote: > On Wed, Jun 27, 2018 at 7:00 AM Kugan Vivekanandarajah > wrote: >> >> Hi Richard, >> >> Thanks for the review. >> >> On 25 June 2018 at 20:01, Richard Biener wrote: >> > On Fri, Jun 22, 2018 at 11:13 AM Kugan Vivekanandarajah >> > 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 >> > type > size >> unit-size >> align:32 warn_if_not_align:0 symtab:0 alias-set 1 >> canonical-type 0x769455e8 precision:32 min > 0x7692dee8 -2147483648> max > 2147483647> >> pointer_to_this > >> >> arg:0 >> >> arg:0 >> visited >> def_stmt a.1_10 = a; >> version:10> >> arg:1 > >> arg:1 >> >> arg:0 > _Bool> >> >> arg:0 > 0x769455e8 int> >> >> arg:0 > 0x769455e8 int> >> arg:0 arg:1 > 0x7694a0d8 -1>> >> arg:1 > 0x769455e8 int> >> visited >> def_stmt c.2_11 = c; >> version:11>> >> arg:1 > 0x769455e8 int> >> visited >> def_stmt b.3_13 = b; >> version:13>> >> arg:1 > int> >> >> arg:0 > 0x769455e8 int> >> >> arg:0 > 0x76a55b28> >> >> arg:0 > 0x76a55b28> >> >> arg:0 > >> arg:0 arg:1 >> >> >> arg:1 > 0x76a55b28> >> arg:0 >>>> >> arg:2 >> >> >> 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 patt
Re: [PATCH 1/3][POPCOUNT] Handle COND_EXPR in expression_expensive_p
On Wed, Jun 27, 2018 at 7:00 AM Kugan Vivekanandarajah wrote: > > Hi Richard, > > Thanks for the review. > > On 25 June 2018 at 20:01, Richard Biener wrote: > > On Fri, Jun 22, 2018 at 11:13 AM Kugan Vivekanandarajah > > 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 > type size > unit-size > align:32 warn_if_not_align:0 symtab:0 alias-set 1 > canonical-type 0x769455e8 precision:32 min 0x7692dee8 -2147483648> max 2147483647> > pointer_to_this > > > arg:0 > > arg:0 > visited > def_stmt a.1_10 = a; > version:10> > arg:1 > > arg:1 > > arg:0 > > arg:0 0x769455e8 int> > > arg:0 0x769455e8 int> > arg:0 arg:1 0x7694a0d8 -1>> > arg:1 0x769455e8 int> > visited > def_stmt c.2_11 = c; > version:11>> > arg:1 0x769455e8 int> > visited > def_stmt b.3_13 = b; > version:13>> > arg:1 int> > > arg:0 0x769455e8 int> > > arg:0 0x76a55b28> > > arg:0 0x76a55b28> > > arg:0 > arg:0 arg:1 > >> > arg:1 0x76a55b28> > arg:0 >>>> > arg:2 >> > > 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 > type size > unit-size > align:32 warn_if_not_align:0 symtab:0 alias-set 1 > canonical-type 0x769455e8 precision:32 min 0x7692dee8 -2147483648> max 2147483647> > pointer_to_this > > side-effects > arg:0 0x769455e8 int> > used ignored SI (null):0:0 size 32> unit-size > align:32 warn_
Re: [PATCH 1/3][POPCOUNT] Handle COND_EXPR in expression_expensive_p
Hi Richard,
Thanks for the review.
On 25 June 2018 at 20:01, Richard Biener wrote:
> On Fri, Jun 22, 2018 at 11:13 AM Kugan Vivekanandarajah
> 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
unit-size
align:32 warn_if_not_align:0 symtab:0 alias-set 1
canonical-type 0x769455e8 precision:32 min max
pointer_to_this >
arg:0
arg:0
visited
def_stmt a.1_10 = a;
version:10>
arg:1 >
arg:1
arg:0
arg:0
arg:0
arg:0 arg:1 >
arg:1
visited
def_stmt c.2_11 = c;
version:11>>
arg:1
visited
def_stmt b.3_13 = b;
version:13>>
arg:1
arg:0
arg:0
arg:0
arg:0
arg:0 arg:1
>>
arg:1
arg:0 >>>>
arg:2 >>
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
unit-size
align:32 warn_if_not_align:0 symtab:0 alias-set 1
canonical-type 0x769455e8 precision:32 min max
pointer_to_this >
side-effects
arg:0
used ignored SI (null):0:0 size unit-size
align:32 warn_if_not_align:0 context >
arg:1
arg:0
arg:0
arg:0
arg:0
arg:0
arg:0 arg:1
>
arg:1
visited
def_stmt c.2_11 = c;
version:11>>>
arg:1
arg:0
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
>>
>> * tree-scalar-evolution.c (expression_expensive_p): Handle COND_EXPR.
From 8f59f05ad21ac218834547593a7f308b4f837b1e Mon Sep 17 00:00:00 2001
From: Kugan Vivekanandarajah
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:
--
2.7.4
Re: [PATCH 1/3][POPCOUNT] Handle COND_EXPR in expression_expensive_p
On Fri, Jun 22, 2018 at 11:13 AM Kugan Vivekanandarajah 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 > > * tree-scalar-evolution.c (expression_expensive_p): Handle COND_EXPR.
[PATCH 1/3][POPCOUNT] Handle COND_EXPR in expression_expensive_p
[PATCH 1/3][POPCOUNT] Handle COND_EXPR in expression_expensive_p
gcc/ChangeLog:
2018-06-22 Kugan Vivekanandarajah
* tree-scalar-evolution.c (expression_expensive_p): Handle COND_EXPR.
From aa38b98dd97567c6032c261f19b3705abc2233b0 Mon Sep 17 00:00:00 2001
From: Kugan Vivekanandarajah
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
