Hi Richard,
On 29 June 2018 at 18:45, Richard Biener <richard.guent...@gmail.com> wrote:
> On Wed, Jun 27, 2018 at 7:09 AM Kugan Vivekanandarajah
> <kugan.vivekanandara...@linaro.org> wrote:
>>
>> Hi Richard,
>>
>> Thanks for the review,
>>
>> On 25 June 2018 at 20:20, Richard Biener <richard.guent...@gmail.com> wrote:
>> > On Fri, Jun 22, 2018 at 11:16 AM Kugan Vivekanandarajah
>> > <kugan.vivekanandara...@linaro.org> wrote:
>> >>
>> >> gcc/ChangeLog:
>> >
>> > @@ -1516,6 +1521,114 @@ minmax_replacement (basic_block cond_bb,
>> > basic_block middle_bb,
>> >
>> > return true;
>> > }
>> > +/* Convert
>> > +
>> > + <bb 2>
>> > + if (b_4(D) != 0)
>> > + goto <bb 3>
>> >
>> > vertical space before the comment.
>> Done.
>>
>> >
>> > + edge e0 ATTRIBUTE_UNUSED, edge e1
>> > ATTRIBUTE_UNUSED,
>> >
>> > why pass them if they are unused?
>> Removed.
>>
>> >
>> > + if (stmt_count != 2)
>> > + return false;
>> >
>> > so what about the case when there is no conversion?
>> Done.
>>
>> >
>> > + /* Check that we have a popcount builtin. */
>> > + if (!is_gimple_call (popcount)
>> > + || !gimple_call_builtin_p (popcount, BUILT_IN_NORMAL))
>> > + return false;
>> > + tree fndecl = gimple_call_fndecl (popcount);
>> > + if ((DECL_FUNCTION_CODE (fndecl) != BUILT_IN_POPCOUNT)
>> > + && (DECL_FUNCTION_CODE (fndecl) != BUILT_IN_POPCOUNTL)
>> > + && (DECL_FUNCTION_CODE (fndecl) != BUILT_IN_POPCOUNTLL))
>> > + return false;
>> >
>> > look at popcount handling in tree-vrp.c how to properly also handle
>> > IFN_POPCOUNT.
>> > (CASE_CFN_POPCOUNT)
>> Done.
>> >
>> > + /* Cond_bb has a check for b_4 != 0 before calling the popcount
>> > + builtin. */
>> > + if (gimple_code (cond) != GIMPLE_COND
>> > + || gimple_cond_code (cond) != NE_EXPR
>> > + || TREE_CODE (gimple_cond_lhs (cond)) != SSA_NAME
>> > + || rhs != gimple_cond_lhs (cond))
>> > + return false;
>> >
>> > The check for SSA_NAME is redundant.
>> > You fail to check that gimple_cond_rhs is zero.
>> Done.
>>
>> >
>> > + /* Remove the popcount builtin and cast stmt. */
>> > + gsi = gsi_for_stmt (popcount);
>> > + gsi_remove (&gsi, true);
>> > + gsi = gsi_for_stmt (cast);
>> > + gsi_remove (&gsi, true);
>> > +
>> > + /* And insert the popcount builtin and cast stmt before the cond_bb. */
>> > + gsi = gsi_last_bb (cond_bb);
>> > + gsi_insert_before (&gsi, popcount, GSI_NEW_STMT);
>> > + gsi_insert_before (&gsi, cast, GSI_NEW_STMT);
>> >
>> > use gsi_move_before (). You need to reset flow sensitive info on the
>> > LHS of the popcount call as well as on the LHS of the cast.
>> Done.
>>
>> >
>> > You fail to check the PHI operand on the false edge. Consider
>> >
>> > if (b != 0)
>> > res = __builtin_popcount (b);
>> > else
>> > res = 1;
>> >
>> > You fail to check the PHI operand on the true edge. Consider
>> >
>> > res = 0;
>> > if (b != 0)
>> > {
>> > __builtin_popcount (b);
>> > res = 2;
>> > }
>> >
>> > and using -fno-tree-dce and whatever you need to keep the
>> > popcount call in the IL. A gimple testcase for phiopt will do.
>> >
>> > Your testcase relies on popcount detection. Please write it
>> > using __builtin_popcount instead. Write one with a cast and
>> > one without.
>> Added the testcases.
>>
>> Is this OK now.
>
> + for (gsi = gsi_start_bb (middle_bb); !gsi_end_p (gsi); gsi_next (&gsi))
> + {
>
> use gsi_after_labels (middle_bb)
>
> + popcount = last_stmt (middle_bb);
> + if (popcount == NULL)
> + return false;
>
> after the counting this test is always false, remove it.
>
> + /* We have a cast stmt feeding popcount builtin. */
> + cast = first_stmt (middle_bb);
>
> looking at the implementation of first_stmt this will
> give you a label in case the BB has one. I think it's better
> to merge this and the above with the "counting" like
>
> gsi = gsi_start_nondebug_after_labels_bb (middle_bb);
> if (gsi_end_p (gsi))
> return false;
> cast = gsi_stmt (gsi);
> gsi_next_nondebug (&gsi);
> if (!gsi_end_p (gsi))
> {
> popcount = gsi_stmt (gsi);
> gsi_next_nondebug (&gsi);
> if (!gsi_end_p (gsi))
> return false;
> }
> else
> {
> popcount = cast;
> cast = NULL;
> }
Done.
>
> + /* Check that we have a cast prior to that. */
> + if (gimple_code (cast) != GIMPLE_ASSIGN
> + || gimple_assign_rhs_code (cast) != NOP_EXPR)
> + return false;
>
> CONVERT_EXPR_CODE_P (gimple_assign_rhs_code (cast))
>
Done.
> + /* Check PHI arguments. */
> + if (lhs != arg0 || !integer_zerop (arg1))
> + return false;
>
> that is not sufficient, you do not know whether arg0 is the true
> value or the false value. The edge flags will tell you.
Done.
Please find the modified patch. Is this OK. Bootstrapped and
regression tested with no new regressions.
Thanks,
Kugan
>
> Otherwise looks OK.
>
> Richard.
>
>> Thanks,
>> Kugan
>> >
>> > Thanks,
>> > Richard.
>> >
>> >
>> >> 2018-06-22 Kugan Vivekanandarajah <kug...@linaro.org>
>> >>
>> >> * tree-ssa-phiopt.c (cond_removal_in_popcount_pattern): New.
>> >> (tree_ssa_phiopt_worker): Call cond_removal_in_popcount_pattern.
>> >>
>> >> gcc/testsuite/ChangeLog:
>> >>
>> >> 2018-06-22 Kugan Vivekanandarajah <kug...@linaro.org>
>> >>
>> >> * gcc.dg/tree-ssa/popcount3.c: New test.
From 1c84a02d568216ecef7ad44f4114ca34f29c537e Mon Sep 17 00:00:00 2001
From: Kugan Vivekanandarajah <kugan.vivekanandara...@linaro.org>
Date: Fri, 22 Jun 2018 14:16:21 +1000
Subject: [PATCH 3/3] improve phiopt for builtin popcount
Change-Id: Ibad062599eb04f96cd582bab34203bcf84273fe9
---
gcc/testsuite/gcc.dg/tree-ssa/phi-opt-16.c | 12 +++
gcc/testsuite/gcc.dg/tree-ssa/phi-opt-17.c | 12 +++
gcc/testsuite/gcc.dg/tree-ssa/phi-opt-18.c | 14 +++
gcc/testsuite/gcc.dg/tree-ssa/phi-opt-19.c | 15 ++++
gcc/testsuite/gcc.dg/tree-ssa/popcount3.c | 15 ++++
gcc/tree-ssa-phiopt.c | 136 +++++++++++++++++++++++++++++
6 files changed, 204 insertions(+)
create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/phi-opt-16.c
create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/phi-opt-17.c
create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/phi-opt-18.c
create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/phi-opt-19.c
create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/popcount3.c
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-16.c b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-16.c
new file mode 100644
index 0000000..e7a2711
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-16.c
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-optimized" } */
+
+int foo (int a)
+{
+ int c = 0;
+ if (a != 0)
+ c = __builtin_popcount (a);
+ return c;
+}
+
+/* { dg-final { scan-tree-dump-not "if" "optimized" } } */
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-17.c b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-17.c
new file mode 100644
index 0000000..25ba096
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-17.c
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-optimized" } */
+
+int foo (unsigned int a)
+{
+ int c = 0;
+ if (a != 0)
+ c = __builtin_popcount (a);
+ return c;
+}
+
+/* { dg-final { scan-tree-dump-not "if" "optimized" } } */
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-18.c b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-18.c
new file mode 100644
index 0000000..cf1aaf5
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-18.c
@@ -0,0 +1,14 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-optimized" } */
+
+int foo (int a)
+{
+ int c = 0;
+ if (a != 0)
+ c = __builtin_popcount (a);
+ else
+ c = 1;
+ return c;
+}
+
+/* { dg-final { scan-tree-dump-times "if " 1 "optimized" } } */
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-19.c b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-19.c
new file mode 100644
index 0000000..1f44066
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-19.c
@@ -0,0 +1,15 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-optimized -fno-tree-dce" } */
+
+int foo (int a)
+{
+ int c = 0;
+ if (a != 0)
+ {
+ __builtin_popcount (a);
+ c = 2;
+ }
+ return c;
+}
+
+/* { dg-final { scan-tree-dump-times "if " 1 "optimized" } } */
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/popcount3.c b/gcc/testsuite/gcc.dg/tree-ssa/popcount3.c
new file mode 100644
index 0000000..293beb9
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/popcount3.c
@@ -0,0 +1,15 @@
+/* { dg-do run } */
+/* { dg-options "-O2 -fdump-tree-phiopt3 -fdump-tree-optimized" } */
+
+int PopCount (long b) {
+ int c = 0;
+
+ while (b) {
+ b &= b - 1;
+ c++;
+ }
+ return c;
+}
+
+/* { dg-final { scan-tree-dump-times "__builtin_popcount" 1 "optimized" } } */
+/* { dg-final { scan-tree-dump-times "if" 0 "phiopt3" } } */
diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
index 8e94f6a..b2575f1 100644
--- a/gcc/tree-ssa-phiopt.c
+++ b/gcc/tree-ssa-phiopt.c
@@ -45,6 +45,7 @@ along with GCC; see the file COPYING3. If not see
#include "tree-scalar-evolution.h"
#include "tree-inline.h"
#include "params.h"
+#include "case-cfn-macros.h"
static unsigned int tree_ssa_phiopt_worker (bool, bool);
static bool conditional_replacement (basic_block, basic_block,
@@ -57,6 +58,8 @@ static bool minmax_replacement (basic_block, basic_block,
edge, edge, gimple *, tree, tree);
static bool abs_replacement (basic_block, basic_block,
edge, edge, gimple *, tree, tree);
+static bool cond_removal_in_popcount_pattern (basic_block, basic_block,
+ edge, edge, gimple *, tree, tree);
static bool cond_store_replacement (basic_block, basic_block, edge, edge,
hash_set<tree> *);
static bool cond_if_else_store_replacement (basic_block, basic_block, basic_block);
@@ -332,6 +335,9 @@ tree_ssa_phiopt_worker (bool do_store_elim, bool do_hoist_loads)
cfgchanged = true;
else if (abs_replacement (bb, bb1, e1, e2, phi, arg0, arg1))
cfgchanged = true;
+ else if (cond_removal_in_popcount_pattern (bb, bb1, e1, e2,
+ phi, arg0, arg1))
+ cfgchanged = true;
else if (minmax_replacement (bb, bb1, e1, e2, phi, arg0, arg1))
cfgchanged = true;
}
@@ -1517,6 +1523,136 @@ minmax_replacement (basic_block cond_bb, basic_block middle_bb,
return true;
}
+/* Convert
+
+ <bb 2>
+ if (b_4(D) != 0)
+ goto <bb 3>
+ else
+ goto <bb 4>
+
+ <bb 3>
+ _2 = (unsigned long) b_4(D);
+ _9 = __builtin_popcountl (_2);
+ OR
+ _9 = __builtin_popcountl (b_4(D));
+
+ <bb 4>
+ c_12 = PHI <0(2), _9(3)>
+
+ Into
+ <bb 2>
+ _2 = (unsigned long) b_4(D);
+ _9 = __builtin_popcountl (_2);
+ OR
+ _9 = __builtin_popcountl (b_4(D));
+
+ <bb 4>
+ c_12 = PHI <_9(2)>
+*/
+
+static bool
+cond_removal_in_popcount_pattern (basic_block cond_bb, basic_block middle_bb,
+ edge e1, edge e2,
+ gimple *phi, tree arg0, tree arg1)
+{
+ gimple *cond;
+ gimple_stmt_iterator gsi, gsi_from;
+ gimple *popcount;
+ gimple *cast = NULL;
+ tree lhs, arg;
+
+ /* Check that
+ _2 = (unsigned long) b_4(D);
+ _9 = __builtin_popcountl (_2);
+ OR
+ _9 = __builtin_popcountl (b_4(D));
+ are the only stmts in the middle_bb. */
+
+ gsi = gsi_start_nondebug_after_labels_bb (middle_bb);
+ if (gsi_end_p (gsi))
+ return false;
+ cast = gsi_stmt (gsi);
+ gsi_next_nondebug (&gsi);
+ if (!gsi_end_p (gsi))
+ {
+ popcount = gsi_stmt (gsi);
+ gsi_next_nondebug (&gsi);
+ if (!gsi_end_p (gsi))
+ return false;
+ }
+ else
+ {
+ popcount = cast;
+ cast = NULL;
+ }
+
+ /* Check that we have a popcount builtin. */
+ if (!is_gimple_call (popcount))
+ return false;
+ combined_fn cfn = gimple_call_combined_fn (popcount);
+ switch (cfn)
+ {
+ CASE_CFN_POPCOUNT:
+ break;
+ default:
+ return false;
+ }
+
+ arg = gimple_call_arg (popcount, 0);
+ lhs = gimple_get_lhs (popcount);
+
+ if (cast)
+ {
+ /* We have a cast stmt feeding popcount builtin. */
+ /* Check that we have a cast prior to that. */
+ if (gimple_code (cast) != GIMPLE_ASSIGN
+ || !CONVERT_EXPR_CODE_P (gimple_assign_rhs_code (cast)))
+ return false;
+ /* Result of the cast stmt is the argument to the builtin. */
+ if (arg != gimple_assign_lhs (cast))
+ return false;
+ arg = gimple_assign_rhs1 (cast);
+ }
+
+ /* Canonicalize. */
+ if (e2->flags & EDGE_TRUE_VALUE)
+ {
+ std::swap (arg0, arg1);
+ std::swap (e1, e2);
+ }
+
+ /* Check PHI arguments. */
+ if (lhs != arg0 || !integer_zerop (arg1))
+ return false;
+
+ cond = last_stmt (cond_bb);
+
+ /* Cond_bb has a check for b_4 != 0 before calling the popcount
+ builtin. */
+ if (gimple_code (cond) != GIMPLE_COND
+ || gimple_cond_code (cond) != NE_EXPR
+ || !integer_zerop (gimple_cond_rhs (cond))
+ || arg != gimple_cond_lhs (cond))
+ return false;
+
+ /* And insert the popcount builtin and cast stmt before the cond_bb. */
+ gsi = gsi_last_bb (cond_bb);
+ if (cast)
+ {
+ gsi_from = gsi_for_stmt (cast);
+ gsi_move_before (&gsi_from, &gsi);
+ reset_flow_sensitive_info (gimple_get_lhs (cast));
+ }
+ gsi_from = gsi_for_stmt (popcount);
+ gsi_move_before (&gsi_from, &gsi);
+ reset_flow_sensitive_info (gimple_get_lhs (popcount));
+
+ /* Now update the PHI and remove unneeded bbs. */
+ replace_phi_edge_with_variable (cond_bb, e2, phi, lhs);
+ return true;
+}
+
/* The function absolute_replacement does the main work of doing the absolute
replacement. Return true if the replacement is done. Otherwise return
false.
--
2.7.4
