Re: [PATCH 3/3][POPCOUNT] Remove unnecessary if condition in phiopt

[Richard Biener <richard dot guenther at gmail dot com>]

On Fri, Jun 22, 2018 at 11:16 AM Kugan Vivekanandarajah
<kugan.vivekanandarajah@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.

+                                 edge e0 ATTRIBUTE_UNUSED, edge e1
ATTRIBUTE_UNUSED,

why pass them if they are unused?

+  if (stmt_count != 2)
+    return false;

so what about the case when there is no conversion?

+  /* 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)

+  /* 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.

+  /* 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.

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.

Thanks,
Richard.


> 2018-06-22  Kugan Vivekanandarajah  <kuganv@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  <kuganv@linaro.org>
>
>     * gcc.dg/tree-ssa/popcount3.c: New test.