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

[Kugan Vivekanandarajah]

Hi Richard,

Thanks for the review,

On 25 June 2018 at 20:20, Richard Biener <richard.guenther@gmail.com> wrote:
> 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.
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.

Thanks,
Kugan
>
> 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-improve-phiopt-for-builtin-popcount.patch
Type: text/x-patch
Size: 8195 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20180627/ef0ae793/attachment.bin>