On Sat, Feb 14, 2026 at 3:48 AM Daniel Barboza
<[email protected]> wrote:
>
> Add a new helper that handles mispredicts in the following bit ops
> scenarios:
>
> - checking if a bitmask is not set, and in this case set it: always set
> the bitmask;
> - checking if a bitmask is set, and in this case clear it: always clear
> the bitmask.
>
> Bootstrapped and tested with x86_64-pc-linux-gnu.
This is NOT a full review, just something which caught my eye.
>
> PR tree-optimization/64567
>
> gcc/ChangeLog:
>
> * tree-ssa-phiopt.cc (block_has_single_assignment): simple
> helper that verifies in a block has a single statement.
> (cond_removal_mispredict_bitop): helper that verifies if we have
> a bitmask check that leads to the same bitmask being
> set/cleared, and make the set/clear unconditional.
> (pass_phiopt::execute): use the new helper.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.dg/tree-ssa/PR64567.c: New test.
> ---
> gcc/testsuite/gcc.dg/tree-ssa/PR64567.c | 23 ++++
> gcc/tree-ssa-phiopt.cc | 170 ++++++++++++++++++++++++
> 2 files changed, 193 insertions(+)
> create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/PR64567.c
>
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/PR64567.c
> b/gcc/testsuite/gcc.dg/tree-ssa/PR64567.c
> new file mode 100644
> index 00000000000..09e8cee62ee
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/PR64567.c
> @@ -0,0 +1,23 @@
> +/* { dg-additional-options -O2 } */
> +/* { dg-additional-options -fdump-tree-phiopt } */
> +
> +#define F1 0x04
> +#define F2 0x08
> +
> +int bar(unsigned flags);
> +
> +int foo(unsigned flags)
> +{
> + if (flags & (F1 | F2))
> + flags &= ~(F1 | F2);
> + return bar(flags);
> +}
> +
> +int baz(unsigned flags)
> +{
> + if (!(flags & F1))
> + flags |= F1;
> + return bar(flags);
> +}
> +
> +/* { dg-final { scan-tree-dump-times " PHI " 0 phiopt2 } } */
> diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
> index fcf44136d0a..ef1c9eb8b19 100644
> --- a/gcc/tree-ssa-phiopt.cc
> +++ b/gcc/tree-ssa-phiopt.cc
> @@ -2703,6 +2703,172 @@ cond_removal_in_builtin_zero_pattern (basic_block
> cond_bb,
> return true;
> }
>
> +/* Check if a BB has a single assignment or a single assignment
> + and a GOTO. Return the gimple assignment or NULL if
> + the BB does not match the criteria. */
> +
> +static gimple*
> +block_has_single_assignment (basic_block bb)
> +{
> + gimple_stmt_iterator gsi = gsi_start_nondebug_after_labels_bb (bb);
if (gsi_end_p (gsi))
return NULL;
> + gimple *stmt = gsi_stmt (gsi);
> +
> + if (!stmt || !is_gimple_assign (stmt))
> + return NULL;
And then you can remove the check for null stmt here.
> +
> + gsi = gsi_last_nondebug_bb (bb);
> + gimple *last_stmt = gsi_stmt (gsi);
> +
> + if (!last_stmt)
> + return NULL;
> +
> + if (gimple_code (last_stmt) == GIMPLE_GOTO)
> + {
> + gsi_prev (&gsi);
> + last_stmt = gsi_stmt (gsi);
> + }
GIMPLE_GOTO only shows up at this stage for computed gotos.
So it should not matter here.
Also maybe it is easier to reuse empty_bb_or_one_feeding_into_p.
So something like:
static gassign*
block_has_single_assignment (basic_block bb, gimple phi)
{
gimple *stmt;
if (!empty_bb_or_one_feeding_into_p (bb, phi, stmt))
return null_ptr;
return safe_dyn_cast<gassign*>(stmt);
}
> +
> + if (last_stmt != stmt)
> + return NULL;
> +
> + return stmt;
> +}
> +
> +/* Optimize the case where we have a bitmask check and
> + a bitmask set/clear of the same mask, making the
> + set/clear unconditional. E.g for a bitmask set case:
> +
> + _1 = flags_3 & bitmask;
> + if (_1 == 0)
> + goto <bb 3>; [INV]
> + else
> + goto <bb 4>; [INV]
> +
> + ;; basic block 3, loop depth 0, maybe hot
> + flags_4 = flags_3 | bitmask;
> + ;; succ: 4 (FALLTHRU,EXECUTABLE)
> +
> + ;; basic block 4, loop depth 0, maybe hot
> + # flags_2 = PHI <flags_3, flags_4>
So we start out with:
a = (a & bitmask) == 0 ? (a | bitmask) : a;
And we want to convert that to:
(a | bitmask)
?
So isn't this just this match pattern:
```
(for (neeq ne eq)
(bitop bit_and bit_ior)
(simplify
(cond (neeq (bit_and @0 @1) integer_zerop)
(bitop@2 @0 @1) @0)
@2))
```
Am I missing something here?
> +
> + We'll make the gcond always true, always setting the
> + bitmask. The gcond disappears, and the BIT_AND op
> + used by it also goes away if it's not being used elsewhere.
> +
> + Likewise for the bitmask clear case. */
> +
> +static bool
> +cond_removal_mispredict_bitop (basic_block cond_bb,
> + basic_block middle_bb,
> + tree arg0, tree arg1)
> +{
> + /* Check if the middle_bb has a single stmt or a
> + single stmt + a goto. */
> + gimple *mid_stmt = block_has_single_assignment (middle_bb);
> + if (!mid_stmt)
> + return false;
> +
> + /* mid_stmt constraints:
> + - Must be either an IOR or an AND;
> + - RHS1 is a SSA_NAME with integral type. */
> + if (gimple_assign_rhs_class (mid_stmt) != GIMPLE_BINARY_RHS)
> + return false;
> +
> + tree rhs1 = gimple_assign_rhs1 (mid_stmt);
> + if (TREE_CODE (rhs1) != SSA_NAME
> + || !INTEGRAL_TYPE_P (TREE_TYPE (rhs1)))
> + return false;
> +
> + tree_code mid_code = gimple_assign_rhs_code (mid_stmt);
> + if (mid_code != BIT_AND_EXPR && mid_code != BIT_IOR_EXPR)
> + return false;
Check mid_code instead of also checking GIMPLE_BINARY_RHS.
> +
> + if (arg0 != gimple_assign_lhs (mid_stmt)
> + || arg1 != gimple_assign_rhs1 (mid_stmt))
> + return false;
> +
> + /* 'cond' must be the format: SSA_NAME EQ|NE integer_zerop,
> + where cond_code varies with mid_stmt OP:
> +
> + - SSA_NAME == 0 and mid_stmt = BIT_AND;
> + - SSA_NAME != 0 and mid_stmt = BIT_IOR.
> +
> + We're assuming that SSA_NAME is a suitable BIT_AND
> + for now. */
> + gcond *cond = safe_dyn_cast <gcond *> (*gsi_last_bb (cond_bb));
Shouldn't this always be a gcond here?
> + if (!cond)
> + return false;
> +
> + if (TREE_CODE (gimple_cond_lhs (cond)) != SSA_NAME
> + || !integer_zerop (gimple_cond_rhs (cond)))
> + return false;
> +
> + tree_code cond_code = gimple_cond_code (cond);
> + if (cond_code != EQ_EXPR && cond_code != NE_EXPR)
> + return false;
> +
> + if ((cond_code == NE_EXPR && mid_code != BIT_AND_EXPR)
> + || (cond_code == EQ_EXPR && mid_code != BIT_IOR_EXPR))
> + return false;
> +
> + gimple *cond_stmt = SSA_NAME_DEF_STMT (gimple_cond_lhs (cond));
> + if (!cond_stmt || !is_gimple_assign (cond_stmt)
> + || gimple_assign_rhs_class (cond_stmt) != GIMPLE_BINARY_RHS
> + || gimple_assign_rhs_code (cond_stmt) != BIT_AND_EXPR)
> + return false;
You don't need to check both rhs_class and rhs_code here. class is
based on the code.
> +
> + /* RHS1 for both cond_stmt and mid_stmt must be the same.
> + RHS2 will depend of what we're trying to map. For a
> + "check if set, if not set it":
> +
> + _1 = SSA_NAME & imm
> + if (_1 == 0) goto 3 else goto 4
> + 3: _4 = SSA_NAME | imm
> + (fallthrough to 4)
> +
> + RHS2 must be the same for both. However for a "if set, clear it"
> + case:
> +
> + _1 = SSA_NAME & imm
> + if (_1 != 0) goto 3 else goto 4
> + 3: _4 = SSA_NAME & (mask ~imm)
> + (fallthrough to 4)
> +
> + mid_stmt RHS2 must clear 'imm', meaning that it must be a
> + mask format. Both RHS2 must also be INTEGER_CST. */
> + if (gimple_assign_rhs1 (cond_stmt) != gimple_assign_rhs1 (mid_stmt))
> + return false;
> +
> + if (mid_code == BIT_IOR_EXPR
> + && gimple_assign_rhs2 (cond_stmt) != gimple_assign_rhs2 (mid_stmt))
> + return false;
> + else if (mid_code == BIT_AND_EXPR)
> + {
> + tree cond_stmt_imm = gimple_assign_rhs2 (cond_stmt);
> + tree mid_stmt_imm = gimple_assign_rhs2 (mid_stmt);
> +
> + if (TREE_CODE (cond_stmt_imm) != INTEGER_CST
> + || TREE_CODE (mid_stmt_imm) != INTEGER_CST)
> + return false;
> +
> + wide_int cond_imm = wi::to_wide (cond_stmt_imm);
> + wide_int mid_imm = wi::to_wide (mid_stmt_imm);
> +
> + if (!wi::ltu_p (cond_imm, mid_imm)
> + || wi::ne_p (mid_imm, wi::bit_not (cond_imm)))
> + return false;
> + }
> +
> + /* Finally set 'cond' to always execute mid_stmt. */
> + edge e = single_pred_edge (middle_bb);
> + if (e->flags & EDGE_TRUE_VALUE)
> + gimple_cond_make_true (cond);
> + else
> + gimple_cond_make_false (cond);
There is a better way of doing this here.
If you move the statement from the middle_bb to right before the conditional,
you could just use replace_phi_edge_with_variable which will do the
right thing.
Thanks,
Andrew Pinski
> +
> + return true;
> +}
> +
> /* Auxiliary functions to determine the set of memory accesses which
> can't trap because they are preceded by accesses to the same memory
> portion. We do that for MEM_REFs, so we only need to track
> @@ -4073,6 +4239,10 @@ pass_phiopt::execute (function *)
> && !diamond_p
> && spaceship_replacement (bb, bb1, e1, e2, phi, arg0, arg1))
> cfgchanged = true;
> + else if (single_pred_p (bb1)
> + && !diamond_p
> + && cond_removal_mispredict_bitop (bb, bb1, arg0, arg1))
> + cfgchanged = true;
> };
>
> execute_over_cond_phis (phiopt_exec);
> --
> 2.51.1
>
