Re: [PATCH v3] gcc/tree-ssa-phiopt.cc: add canonicalize_phi_constants [PR71336]

On Sun, Jul 26, 2026 at 2:37 AM Daniel Barboza
<[email protected]> wrote:
>
> This was done previously in match.pd ([1] is the last match.pd version)
> but I grew to dislike how that turned out.  We we relying on assumptions
> like "we are counting on something else to undo the transformation we
> are doing since it would be better".  The reason is that, in match.pd, we
> can't capture the resulting LHS of the simplification to run checks on
> it (e.g. if it's single use).  In particular we want to detect if the
> phi result would be used in 122608 and do not get in the way.  We can
> do these checks in the passes though, thus I decided to move the logic
> out of match.pd.
>
> The idea is the same: identify cases where a zero_one comparison is used
> to conditional constant assignment and turn that into an unconditional
> PLUS.  For the code in PR71336:
>
> int test(int a) {
>     return a & 1 ? 7 : 3;
> }
>
> We'll turn that into "(a&1) * (7 - 3) + 3", which yields the same
> results but without the conditional, promoving more optimization
> opportunities.  We're also handling the "zero_one EQ 0" variant by
> canonicalizing it to "zero_one NE 0".

Thinking about this is we really want this in expand and/or ifcvt
rather than gimple I think.
I do think simple:
if (a != 0)
  b = c;
else
  b = d;
Should be converted into:
t = a != 0;
b = t ? c : d;
during the last phiopt.
But that requires us to add more to expand to support all of the tricks.
From the old review of my patch to add some of the tricks to expand,
RTH mentioned the constant cases should be left up to the target but
that was ~10 years ago. I am suspect it would be best if it is left up
to the targets for constants though we need to start adding tricks to
the target movcc patterns then.

>
> x86_64 and aarch64 tests show better code all around.  riscv64 shows
> code improvement in the NE pattern but a small code regression in the EQ
> case.  Jeff is aware of it and told that we should go ahead with the
> transformation since other targets benefit from it.
>
> Bootstrapped and regression tested in x86_64, aarch64 and riscv64.
>
> [1] https://gcc.gnu.org/pipermail/gcc-patches/2026-March/710154.html
>
>         PR tree-optimization/71336
>
> gcc/ChangeLog:
>
>         * tree-ssa-phiopt.cc (canonicalize_phi_constants): transform
>         "zero_one NE|EQ 0 ? CST1 : CST2" into "CST + zero_one*diff".
>         (pass_phiopt::execute): use canonicalize_phi_constants.
>
> gcc/testsuite/ChangeLog:
>
>         * gcc.dg/tree-ssa/pr71336-2.c: New test.
>         * gcc.dg/tree-ssa/pr71336.c: New test.
> ---
>
> Changes from v2:
> - changed from match.pd to tree-ssa-phiopt;
> - v2 link: https://gcc.gnu.org/pipermail/gcc-patches/2026-March/710154.html
>
>  gcc/testsuite/gcc.dg/tree-ssa/pr71336-2.c |  63 +++++++
>  gcc/testsuite/gcc.dg/tree-ssa/pr71336.c   |  22 +++
>  gcc/tree-ssa-phiopt.cc                    | 198 ++++++++++++++++++++++
>  3 files changed, 283 insertions(+)
>  create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr71336-2.c
>  create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr71336.c
>
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr71336-2.c b/gcc/testsuite/gcc.dg/tree-ssa/pr71336-2.c
> new file mode 100644
> index 00000000000..3af80507234
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr71336-2.c
> @@ -0,0 +1,63 @@
> +/* { dg-do run } */
> +/* { dg-options "-O1" } */
> +
> +/* Macro adapted from builtin-object-size-common.h  */
> +#define FAIL() \
> +  do { \
> +    __builtin_printf ("Failure at line: %d\n", __LINE__);     \
> +    abort();                                                 \
> +  } while (0)
> +
> +void abort(void);
> +
> +int test (int a) {
> +    return a & 1 ? 7 : 3;
> +}
> +
> +int test2 (int a) {
> +    return a & 1 ? 3 : 7;
> +}
> +
> +int test3 (int a) {
> +    return (a & 1) == 0 ? 3 : 7;
> +}
> +
> +int test4 (int a) {
> +    return (a & 1) == 0 ? 7 : 3;
> +}
> +
> +int main (void) {
> +  /* a & 1 ? 7 : 3;  */
> +  if (test (0) != 3)
> +    FAIL ();
> +  if (test (1) != 7)
> +    FAIL ();
> +  if (test (3) != 7)
> +    FAIL ();
> +
> +  /* a & 1 ? 3 : 7;  */
> +  if (test2 (0) != 7)
> +    FAIL ();
> +  if (test2 (1) != 3)
> +    FAIL ();
> +  if (test2 (3) != 3)
> +    FAIL ();
> +
> +  /* (a & 1) == 0 ? 3 : 7;  */
> +  if (test3 (0) != 3)
> +    FAIL ();
> +  if (test3 (1) != 7)
> +    FAIL ();
> +  if (test3 (2) != 3)
> +    FAIL ();
> +
> +  /* (a & 1) == 0 ? 7 : 3;  */
> +  if (test4 (0) != 7)
> +    FAIL ();
> +  if (test4 (1) != 3)
> +    FAIL ();
> +  if (test4 (2) != 7)
> +    FAIL ();
> +
> +  return 0;
> +}
> \ No newline at end of file
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr71336.c b/gcc/testsuite/gcc.dg/tree-ssa/pr71336.c
> new file mode 100644
> index 00000000000..1a0c30baf70
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr71336.c
> @@ -0,0 +1,22 @@
> +/* { dg-additional-options -O1 } */
> +/* { dg-additional-options -fdump-tree-optimized } */
> +
> +int test (int a) {
> +    return a & 1 ? 17 : 3;
> +}
> +
> +int test2 (int a) {
> +    return (a & 1) == 0 ? 17 : 3;
> +}
> +
> +int test3 (int a) {
> +    return a & 1 ? 3 : 17;
> +}
> +
> +int test4 (int a) {
> +    return (a & 1) == 0 ? 3 : 17;
> +}
> +
> +/* { dg-final { scan-tree-dump-times " \\* " 4 optimized } } */
> +/* { dg-final { scan-tree-dump-times " goto " 0 optimized } } */
> +
> \ No newline at end of file
> diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
> index d48f164c7fb..f209c673f15 100644
> --- a/gcc/tree-ssa-phiopt.cc
> +++ b/gcc/tree-ssa-phiopt.cc
> @@ -3906,6 +3906,198 @@ cond_if_else_store_replacement (basic_block then_bb, basic_block else_bb,
>    return ok;
>  }
>
> +/* Given a PHI with 2 edges, both with CST args and an
> +   empty middle_bb, with a zero_one NE zero comparison:
> +
> +   <bb 2>
> +   if (zero_one != 0) goto <bb 4> else goto <bb 3>
> +   <bb 3>
> +   goto <bb 4>
> +   <bb 4>
> +   c_12 = PHI <3 (3), 17 (2)>
> +
> +   Canonizalize it into a PHI <0,1> that recreates the CSTs
> +   with a MULT:
> +
> +   <bb 2>
> +   if (zero_one != 0) goto <bb 4> else goto <bb 3>
> +   <bb 3>
> +   goto <bb 4>
> +   <bb 4>
> +   c_12 = PHI <0 (3), 1 (2)>
> +   _ssa1 = c_12 * (17 - 3)
> +   _ssa2 = 3 + _ssa1
> +   (replace c_12 with _ssa2)
> +
> +   This allows the following phiopt pass (via match_simplify_replacement)
> +   to eliminate the gcond and the middle_bb, resulting in better code
> +   generation.
> +
> +   EQ comparisons are also supported and will canonicalized to NE.
> +
> +   The canonicalization is restricted to zero_one comparisons because
> +   match.pd has forwprop transformations such as:
> +
> +   (m1 cmp m2) * d => (m1 cmp m2) ? d : 0
> +
> +   That would end up undoing the canonicalization done here.  */
> +static bool
> +canonicalize_phi_constants (basic_block cond_bb, gphi *phi,
> +                           tree arg0, tree arg1,
> +                           edge e0, edge e1)
> +{
> +  if (gimple_phi_num_args (phi) != 2
> +      || TREE_CODE (arg0) != INTEGER_CST
> +      || TREE_CODE (arg1) != INTEGER_CST
> +      || tree_int_cst_sgn (arg0) <= 0
> +      || tree_int_cst_sgn (arg1) <= 0
> +      || !tree_fits_uhwi_p (arg0)
> +      || !tree_fits_uhwi_p (arg1)
> +      || (tree_to_uhwi (arg0) == tree_to_uhwi (arg1)))
> +    return false;
> +
> +  tree phires = gimple_phi_result (phi);
> +
> +  /* ??? Do we need to check both virtual_operand_p and !INTEGRAL_TYPE_P?  */
> +  if (virtual_operand_p (phires)
> +      || !INTEGRAL_TYPE_P (TREE_TYPE (phires)))
> +    return false;
> +
> +  /* Check if phi_res is single use and not used in any
> +     binary operation.  We make this check to avoid getting
> +     in the way of simplifications such as PR 122608 that
> +     are better.  */
> +  use_operand_p use;
> +  gimple *use_stmt;
> +  if (!single_imm_use (phires, &use, &use_stmt)
> +      || (!is_a<gassign*> (use_stmt)
> +         && !is_a<greturn*> (use_stmt))
> +      || (is_a<gassign*> (use_stmt)
> +         && get_gimple_rhs_class (
> +               gimple_assign_rhs_code (use_stmt)) == GIMPLE_BINARY_RHS))
> +    return false;
> +
> +  gcond *cond = as_a <gcond *> (*gsi_last_bb (cond_bb));
> +  tree_code cond_code = gimple_cond_code (cond);
> +  tree zero_one = gimple_cond_lhs (cond);
> +  if (!tree_zero_one_valued_p (zero_one)
> +      || !INTEGRAL_TYPE_P (TREE_TYPE (zero_one))
> +      || !integer_zerop (gimple_cond_rhs (cond))
> +      || (cond_code != NE_EXPR && cond_code != EQ_EXPR))
> +    return false;
> +
> +  /* At this point we're committed.  What we want now is:
> +     - if we have an EQ_EXPR canonicalize it to NE_EXPR to
> +       simplify the logic;
> +     - add a "new_phires = PHI <0, 1>;" gphi;
> +     - add a "new_phires * (CST_GT - CST_LT)" stmt;
> +     - add a "CST PLUS (new_phires*diff)" stmt;
> +     - replace phires with new_phires and remove the old PHI.  */
> +
> +  bool cond_canonicalized = false;
> +  if (cond_code == EQ_EXPR)
> +    {
> +      gimple_cond_set_code (cond, NE_EXPR);
> +      std::swap (arg0, arg1);
> +      cond_canonicalized = true;
> +    }
> +
> +  unsigned HOST_WIDE_INT diff = 0;
> +  unsigned HOST_WIDE_INT arg0_val = tree_to_uhwi (arg0);
> +  unsigned HOST_WIDE_INT arg1_val = tree_to_uhwi (arg1);
> +  bool arg0_gt = false;
> +
> +  if (arg0_val > arg1_val)
> +    {
> +      diff = arg0_val - arg1_val;
> +      arg0_gt = true;
> +    }
> +  else
> +    diff = arg1_val - arg0_val;
> +
> +  bool e0_true_edge = false;
> +  if (e0->flags & EDGE_TRUE_VALUE)
> +    e0_true_edge = true;
> +
> +  tree cst_stmt_operand;
> +
> +  /* zero_one NE 0 ? CST_GT : CST_LT will be reduced to
> +     CST_LT + zero_one*diff; */
> +  if ((e0_true_edge && arg0_gt)
> +       || (!e0_true_edge && !arg0_gt))
> +    {
> +      cst_stmt_operand = arg0_gt ? arg1 : arg0;
> +    }
> +  /* zero_one NE 0 ? CST_LT : CST_GT will be reduced to
> +     CST_GT + zero_one*(-diff); */
> +  else if ((e0_true_edge && !arg0_gt)
> +           || (!e0_true_edge && arg0_gt))
> +    {
> +      diff = -diff;
> +      cst_stmt_operand = arg0_gt ? arg0 : arg1;
> +    }
> +  else
> +    gcc_unreachable ();
> +
> +  tree elems_type = TREE_TYPE (cst_stmt_operand);
> +  tree new_phires = make_ssa_name (elems_type, NULL);
> +  gphi *new_phi = create_phi_node (new_phires, phi->bb);
> +
> +  /* For zero_one NE 0 ? CST1 : CST2, zero_one == 1
> +     in the 'true' edge.  */
> +  tree e0_arg, e1_arg;
> +  if (e0_true_edge)
> +    {
> +      e0_arg = build_int_cst (elems_type, 1);
> +      e1_arg = build_int_cst (elems_type, 0);
> +    }
> +  else
> +    {
> +      e0_arg = build_int_cst (elems_type, 0);
> +      e1_arg = build_int_cst (elems_type, 1);
> +    }
> +
> +  SET_PHI_ARG_DEF (new_phi, e0->dest_idx, e0_arg);
> +  SET_PHI_ARG_DEF (new_phi, e1->dest_idx, e1_arg);
> +
> +  gimple_seq seq = nullptr;
> +
> +  /* new_phires * diff stmt.  */
> +  tree mult_lhs = gimple_build (&seq, MULT_EXPR, elems_type,
> +       new_phires, build_int_cst (elems_type, diff));
> +  /* CST PLUS (new_phires*diff) stmt.  */
> +  tree cst_lhs = gimple_build (&seq, PLUS_EXPR, elems_type,
> +       cst_stmt_operand, mult_lhs);
> +
> +  /* In theory we could do replace_phi_edge_with_variable here and
> +     be done with it but bootstrap really dislikes that.  In the
> +     next phiopt pass match_simplify_replacement will replace the
> +     phi and make a better job at it, so for now we're happy
> +     with just adjusting the new PHI and the extra stmts.  */
> +  gimple_stmt_iterator gsi = gsi_start_bb (phi->bb);
> +  gsi_insert_seq_before (&gsi, seq, GSI_CONTINUE_LINKING);
> +
> +  replace_uses_by (phires, cst_lhs);
> +
> +  gsi = gsi_for_phi (phi);
> +  gsi_remove (&gsi, true);
> +
> +  statistics_counter_event (cfun, "Canonicalized PHI constant args", 1);
> +  if (dump_file && (dump_flags & TDF_DETAILS))
> +    {
> +      if (cond_canonicalized)
> +       fprintf (dump_file,
> +                "COND_EXPR in block %d canonicalized to NE_EXPR. ",
> +                cond_bb->index);
> +
> +      fprintf (dump_file,
> +              "Constant PHI args in block %d canonicalized to 0/1.\n",
> +              new_phi->bb->index);
> +    }
> +
> +  return true;
> +}
> +
>  /* If PHI at MERGE is a "load PHI", PHI <*P, *Q> whose two arguments are
>     single-use, non-volatile scalar MEM_REF loads reading the same memory state
>     (same VUSE), factor the load out: introduce P' = PHI <P, Q> and a single
> @@ -4756,6 +4948,12 @@ pass_phiopt::execute (function *)
>                && !diamond_p
>                && spaceship_replacement (bb, bb1, e1, e2, phi, arg0, arg1))
>         cfgchanged = true;
> +      else if (!early_p
> +              && !diamond_p
> +              && single_pred_p (bb1)
> +              && empty_block_p (bb1)
> +              && canonicalize_phi_constants (bb, phi, arg0, arg1, e1, e2))
> +       cfgchanged = true;
>      };
>
>    execute_over_cond_phis (phiopt_exec);
> --
> 2.43.0
>