On Sat, Oct 24, 2026 at 6:57 AM Andrea Pinski
<[email protected]> wrote:
>
> This adds support to match_simplify_replacement to allowing of the
> middle-bbs to have more than 1 statement in a few cases. If we are
> replacing the both middle bbs with a fully independent sequence we
> can handle that. If we are replacing with a sequence that depends
> on the first statement of either (or both) blocks, this allows that.
> If the result is defined by part or fully of one of the middle bbs.
> Then make the whole bb unconditional.
>
> Note the middle bbs still need to have statements that can't trap nor have
> side effects. And each of the statements need to feed into the next
> instruction and including into the phi. And there is a limited number of
> stmts is allowed. Also have an allow list of expressions type to be used
> after the first few stmts.
>
> This allows removal of the manual matching done in
> cond_removal_in_builtin_zero_pattern as those have been
> moved to match patterns back in r14-362-gc53237cefbad0c
> and r14-414-g078339fc04f5de.
>
> Changes since v1:
> * v2: Add allow list to feeding_bb_info and add a limit to
> the number of stmts to consider. Adds 2 testcases based on 2 pre-existing
> match patterns to show what can be done.
> * v3: Use walk_gimple_op instead of gimple_op walk. Add check for some
> possible undefined code when becoming unconditional (shifts, rotates,
> clz/ctz); add testcases for that too. Fix allow expressions detection
> and calls to be better.
> * v4: Fix up shift undefined behavior slightly more.
>
> Note Sashiko LLM analysis is not clean because the cases it is complaining
> about will be rejected later on or won't show up (e.g. CLZ with a one bit
> argument is folded aways or a shift for one bit will also be simplified away).
> The code left behind in one of the middle bb is dead; the patch which was
> posted earlier today will fix up those dead stmts; the ICE it mentions also
> will never show up because simple_dce will not delete stmts that are in use
> still and the phi will have been changed to reference only the ones from the
> condition (and dominating bb) and the dominator is not updated until
> cfgcleanup even so messes up that analysis too.
>
> Bootstrapped and tested on x86_64-linux-gnu.
>
> gcc/ChangeLog:
>
> * tree-ssa-phiopt.cc (gsi_next_nondebug_nonnop_non_predict): New
> function.
> (enum bbfeeding_info): New enum.
> (empty_bb_or_one_feeding_into_p): Rename to ...
> (feeding_bb_info): This and support few different other cases.
> (auto_flow_sensitive): Increase the vect base size to 4.
> (auto_flow_sensitive::auto_flow_sensitive(gimple*)): Split out into
> ...
> (auto_flow_sensitive::save_stmt): This.
> (auto_flow_sensitive::auto_flow_sensitive(basic_block)): New
> constructor.
> (match_simplify_replacement): Support More than one statement in
> the middle BB.
> * params.opt (-param=phiopt-max-feeding-stmts=): New option.
> * doc/params.texi (phiopt-max-feeding-stmts): Document.
>
> PR tree-optimization/127166
>
> gcc/testsuite/ChangeLog:
>
> * gcc.dg/tree-ssa/ssa-dom-thread-16.c: Disable phiopt.
> * gcc.dg/tree-ssa/clamp-1.c: New test.
> * gcc.dg/tree-ssa/umax-sub-1.c: New test.
> * gcc.dg/tree-ssa/and-ctz-1.c: New test.
> * gcc.dg/tree-ssa/and-shift-1.c: New test.
>
> Signed-off-by: Andrea Pinski <[email protected]>
> ---
> gcc/doc/params.texi | 5 +
> gcc/params.opt | 4 +
> gcc/testsuite/gcc.dg/tree-ssa/and-ctz-1.c | 21 +
> gcc/testsuite/gcc.dg/tree-ssa/and-shift-1.c | 14 +
> gcc/testsuite/gcc.dg/tree-ssa/clamp-1.c | 25 ++
> .../gcc.dg/tree-ssa/ssa-dom-thread-16.c | 2 +-
> gcc/testsuite/gcc.dg/tree-ssa/umax-sub-1.c | 14 +
> gcc/tree-ssa-phiopt.cc | 421 ++++++++++++++----
> 8 files changed, 415 insertions(+), 91 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/and-ctz-1.c
> create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/and-shift-1.c
> create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/clamp-1.c
> create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/umax-sub-1.c
>
> diff --git a/gcc/doc/params.texi b/gcc/doc/params.texi
> index 090af64a7e7..b26513a7ea1 100644
> --- a/gcc/doc/params.texi
> +++ b/gcc/doc/params.texi
> @@ -57,6 +57,11 @@ When factoring statements out of if/then/else, this is the
> maximum number
> of statements
> after the defining statement to be allowed to extend the lifetime of a name.
>
> +@paindex phiopt-max-feeding-stmts
> +@item phiopt-max-feeding-stmts
> +When deciding if a basic block is too big to phiopt, this decides the maximum
> +number of statements that feed into one another is too many. This excludes
> converts.
> +
> @paindex predictable-branch-outcome
> @item predictable-branch-outcome
> When branch is predicted to be taken with probability lower than this
> threshold
> diff --git a/gcc/params.opt b/gcc/params.opt
> index 29a5c7018a8..f0702f97625 100644
> --- a/gcc/params.opt
> +++ b/gcc/params.opt
> @@ -958,6 +958,10 @@ Maximum probability of the entry BB of split region (as
> a percentage relative to
> Common Joined UInteger Var(param_phiopt_factor_max_stmts_live) Init(5)
> Optimization IntegerRange(0, 100) Param
> Maximum number of statements allowed between a defining statement and the
> end of the basic block before extending the live range is considered
> profitable.
>
> +-param=phiopt-max-feeding-stmts=
> +Common Joined UInteger Var(param_phiopt_max_feeding_stmts) Init(4)
> Optimization IntegerRange(1, 100) Param
> +Maximum number of statements can considered for each basic block excluding
> converts.
> +
> -param=predictable-branch-outcome=
> Common Joined UInteger Var(param_predictable_branch_outcome) Init(2)
> IntegerRange(0, 50) Param Optimization
> Maximal estimated outcome of branch considered predictable.
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/and-ctz-1.c
> b/gcc/testsuite/gcc.dg/tree-ssa/and-ctz-1.c
> new file mode 100644
> index 00000000000..4408addfe3a
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/and-ctz-1.c
> @@ -0,0 +1,21 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fdump-tree-phiopt2-details -fdump-tree-optimized" } */
> +
> +int f(int a, int b)
> +{
> + if (a == 0)
> + return 0;
> + return a & __builtin_ctz(b);
> +}
> +
> +int f1(int a, int b)
> +{
> + if (a == 0)
> + return 0;
> + return a & __builtin_clz(b);
> +}
> +
> +/* Here we can't make the call to ctz/clz unconditional without undefined
> behavior. */
> +
> +/* { dg-final { scan-tree-dump-not "converted to straightline code"
> "phiopt2" } } */
> +/* { dg-final { scan-tree-dump-times "if " 2 "optimized" } } */
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/and-shift-1.c
> b/gcc/testsuite/gcc.dg/tree-ssa/and-shift-1.c
> new file mode 100644
> index 00000000000..f0e96e0b0d6
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/and-shift-1.c
> @@ -0,0 +1,14 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fdump-tree-phiopt2-details -fdump-tree-optimized" } */
> +
> +int f(int a, int b, int c)
> +{
> + if (a == 0)
> + return 0;
> + return a & (b >> c);
> +}
> +
> +/* Here we can't make the shift unconditional without undefined behavior. */
> +
> +/* { dg-final { scan-tree-dump-not "converted to straightline code"
> "phiopt2" } } */
> +/* { dg-final { scan-tree-dump "if " "optimized" } } */
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/clamp-1.c
> b/gcc/testsuite/gcc.dg/tree-ssa/clamp-1.c
> new file mode 100644
> index 00000000000..43239772ca0
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/clamp-1.c
> @@ -0,0 +1,25 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fdump-tree-phiopt2-details -fdump-tree-optimized" } */
> +
> +/* PR tree-optimization/127166 */
> +
> +unsigned char clip(int x)
> +{
> + unsigned u;
> + unsigned char r;
> + u = x;
> + r = x;
> + if (u > 255)
> + {
> + int n = -x;
> + int s = n >> (sizeof(int)*8 - 1);
> + r = s;
> + }
> + return r;
> +}
> +
> +
> +/* { dg-final { scan-tree-dump "converted to straightline code" "phiopt2" }
> } */
> +/* { dg-final { scan-tree-dump "MAX_EXPR" "phiopt2" } } */
> +/* { dg-final { scan-tree-dump "MIN_EXPR" "phiopt2" } } */
> +/* { dg-final { scan-tree-dump-not "if " "optimized" } } */
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/ssa-dom-thread-16.c
> b/gcc/testsuite/gcc.dg/tree-ssa/ssa-dom-thread-16.c
> index e8555f2d963..51b686b1de8 100644
> --- a/gcc/testsuite/gcc.dg/tree-ssa/ssa-dom-thread-16.c
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/ssa-dom-thread-16.c
> @@ -1,5 +1,5 @@
> /* { dg-do compile } */
> -/* { dg-options "-O2 -fdump-tree-dom2-details -w --param
> logical-op-non-short-circuit=1 -fdisable-tree-threadfull1" } */
> +/* { dg-options "-O2 -fdump-tree-dom2-details -w --param
> logical-op-non-short-circuit=1 -fdisable-tree-threadfull1 -fno-ssa-phiopt" }
> */
> unsigned char
> validate_subreg (unsigned int offset, unsigned int isize, unsigned int
> osize, int zz, int qq)
> {
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/umax-sub-1.c
> b/gcc/testsuite/gcc.dg/tree-ssa/umax-sub-1.c
> new file mode 100644
> index 00000000000..c07eb4154d6
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/umax-sub-1.c
> @@ -0,0 +1,14 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fdump-tree-phiopt2-details -fdump-tree-optimized" } */
> +
> +signed f1 (signed x)
> +{
> + if (x == 0) return 0;
> + unsigned y = x;
> + y -= 1;
> + x = y;
> + return x;
> +}
> +
> +/* { dg-final { scan-tree-dump "converted to straightline code" "phiopt2" }
> } */
> +/* { dg-final { scan-tree-dump-not "if " "optimized" } } */
> diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
> index 53256d1f787..08b40897a60 100644
> --- a/gcc/tree-ssa-phiopt.cc
> +++ b/gcc/tree-ssa-phiopt.cc
> @@ -56,6 +56,7 @@ along with GCC; see the file COPYING3. If not see
> #include "gimple-predict.h"
> #include "alias.h"
> #include "tree-ssa-threadedge.h"
> +#include "gimple-walk.h"
>
> /* Return the singleton PHI in the SEQ of PHIs for edges E0 and E1. */
>
> @@ -832,6 +833,19 @@ gimple_simplify_phiopt (bool early_p, tree type, gimple
> *comp_stmt,
> return NULL;
> }
>
> +/* Like gsi_next_nondebug but also skips over NOPs and predicats. */
predicts.
> +static void
> +gsi_next_nondebug_nonnop_nonpredict (gimple_stmt_iterator *i)
> +{
> + do
> + {
> + gsi_next_nondebug (i);
> + }
> + while (!gsi_end_p (*i)
> + && (gimple_code (**i) == GIMPLE_PREDICT
> + || gimple_code (**i) == GIMPLE_NOP));
> +}
> +
> /* one_feeding_comparison_into_p returns true if BB has one comparison
> statement and it sets STMT to that statement. Note the comparison can
> be trapping too. */
> @@ -896,110 +910,191 @@ one_feeding_comparison_into_p (basic_block bb,
> return true;
> }
>
> -/* empty_bb_or_one_feeding_into_p returns true if bb was empty basic block
> - or it has one cheap preparation statement that feeds into the PHI
> - statement and it sets STMT to that statement. */
> -static bool
> -empty_bb_or_one_feeding_into_p (basic_block bb,
> - gimple *phi,
> - gimple *&stmt)
> +
> +enum bbfeeding_info
> +{
> + invalid_bb,
> + empty_bb,
> + one_stmt_bb,
> + feeding_bb
> +};
> +
> +/* This returns empty_bb if bb was empty basic block.
> + or returns one_stmt_bb if it has one cheap preparation
> + statement that feeds into the PHI statement and it sets
> + STMT to that statement. Returns feeding_bb if the
> + basic block is not empty but has a sequence of statements
> + feeding into the phi that has no side effects. */
> +static bbfeeding_info
> +feeding_bb_info (basic_block bb, gimple *phi, gimple *&stmt)
> {
> stmt = nullptr;
> gimple *stmt_to_move = nullptr;
> tree lhs;
>
> if (empty_block_p (bb))
> - return true;
> + return empty_bb;
>
> if (!single_pred_p (bb))
> - return false;
> + return invalid_bb;
>
> /* The middle bb cannot have phi nodes as we don't
> move those assignments yet. */
> if (!gimple_seq_empty_p (phi_nodes (bb)))
> - return false;
> + return invalid_bb;
>
> gimple_stmt_iterator gsi;
>
> gsi = gsi_start_nondebug_after_labels_bb (bb);
> + int stmts = 0;
> + int stmts_minus_convert = 0;
> + bool non_allowed_stmt = false;
> + bool had_undefined_stmt = false;
> + bool has_call = false;
> while (!gsi_end_p (gsi))
> {
> - gimple *s = gsi_stmt (gsi);
> - gsi_next_nondebug (&gsi);
> - /* Skip over Predict and nop statements. */
> + gimple *s = *gsi;
> + gsi_next_nondebug_nonnop_nonpredict (&gsi);
> + // Skip over predict and nop statements which could happen
> + // if they are at the beginging.
> if (gimple_code (s) == GIMPLE_PREDICT
> || gimple_code (s) == GIMPLE_NOP)
> continue;
isn't that now redundant? Ah, seeing below use of gsi
this becomes clear.
> - /* If there is more one statement return false. */
> - if (stmt_to_move)
> - return false;
> - stmt_to_move = s;
> - }
> -
> - /* The only statement here was a Predict or a nop statement
> - so return true. */
> - if (!stmt_to_move)
> - return true;
> -
> - if (gimple_vuse (stmt_to_move))
> - return false;
> -
> - if (gimple_could_trap_p (stmt_to_move)
> - || gimple_has_side_effects (stmt_to_move))
> - return false;
> -
> - ssa_op_iter it;
> - tree use;
> - FOR_EACH_SSA_TREE_OPERAND (use, stmt_to_move, it, SSA_OP_USE)
> - if (ssa_name_maybe_undef_p (use))
> - return false;
> -
> - /* Allow assignments but allow some builtin/internal calls.
> - As const calls don't match any of the above, yet they could
> - still have some side-effects - they could contain
> - gimple_could_trap_p statements, like floating point
> - exceptions or integer division by zero. See PR70586.
> - FIXME: perhaps gimple_has_side_effects or gimple_could_trap_p
> - should handle this.
> - Allow some known builtin/internal calls that are known not to
> - trap: logical functions (e.g. bswap and bit counting). */
> - if (!is_gimple_assign (stmt_to_move))
> - {
> - if (!is_gimple_call (stmt_to_move))
> - return false;
> - combined_fn cfn = gimple_call_combined_fn (stmt_to_move);
> - switch (cfn)
> + stmts++;
> + stmts_minus_convert++;
> + if (stmts_minus_convert > param_phiopt_max_feeding_stmts)
> + return invalid_bb;
> + if (gimple_vuse (s))
> + return invalid_bb;
> + if (gimple_could_trap_p (s)
> + || gimple_has_side_effects (s))
I think throw_external doesn't count as side-effect, so you'd want to
check that as well I guess. Maybe the checks below cover this.
> + return invalid_bb;
> + ssa_op_iter it;
> + tree use;
> + FOR_EACH_SSA_TREE_OPERAND (use, s, it, SSA_OP_USE)
> + if (ssa_name_maybe_undef_p (use))
> + return invalid_bb;
> +
> + /* Allow some assignments but allow some builtin/internal calls.
> + As const calls don't match any of the above, yet they could
> + still have some side-effects - they could contain
> + gimple_could_trap_p statements, like floating point
> + exceptions or integer division by zero. See PR70586.
> + FIXME: perhaps gimple_has_side_effects or gimple_could_trap_p
> + should handle this.
> + Allow some known builtin/internal calls that are known not to
> + trap: logical functions (e.g. bswap and bit counting). */
> + if (!is_gimple_assign (s))
> {
> - default:
> - return false;
> - CASE_CFN_BSWAP:
> - CASE_CFN_BITREVERSE:
> - CASE_CFN_FFS:
> - CASE_CFN_PARITY:
> - CASE_CFN_POPCOUNT:
> - CASE_CFN_CLZ:
> - CASE_CFN_CTZ:
> - case CFN_BUILT_IN_CLRSB:
> - case CFN_BUILT_IN_CLRSBL:
> - case CFN_BUILT_IN_CLRSBLL:
> - lhs = gimple_call_lhs (stmt_to_move);
> - break;
> + if (!is_gimple_call (s))
I wonder if a switch (gimple_code (s)) would make this
easier to follow
> + return invalid_bb;
> + combined_fn cfn = gimple_call_combined_fn (s);
> + switch (cfn)
> + {
> + default:
> + return invalid_bb;
> + CASE_CFN_CLZ:
> + CASE_CFN_CTZ:
> + if (gimple_call_num_args (s) != 2)
> + had_undefined_stmt = true;
> + // FALLTHRU
> + case CFN_BUILT_IN_CLRSB:
> + case CFN_BUILT_IN_CLRSBL:
> + case CFN_BUILT_IN_CLRSBLL:
> + CASE_CFN_BSWAP:
> + CASE_CFN_BITREVERSE:
> + CASE_CFN_FFS:
> + CASE_CFN_PARITY:
> + CASE_CFN_POPCOUNT:
> + lhs = gimple_call_lhs (s);
> + // A call can after some stmts but not with a non allowed one
> + // or another call.
> + if (non_allowed_stmt || has_call)
> + return invalid_bb;
> + has_call = true;
> + break;
> + }
> }
> + else
> + {
> + lhs = gimple_assign_lhs (s);
> + // Only allow a few different for the feeding list.
> + switch (gimple_assign_rhs_code (s))
> + {
> + // Always allow a convert.
> + CASE_CONVERT:
> + stmts_minus_convert --;
> + break;
> + case PLUS_EXPR:
> + case MINUS_EXPR:
> + case ABS_EXPR:
> + case ABSU_EXPR:
> + case BIT_IOR_EXPR:
> + case BIT_XOR_EXPR:
> + case BIT_AND_EXPR:
> + case MIN_EXPR:
> + case MAX_EXPR:
> + case NEGATE_EXPR:
> + break;
> + // Shifts with a (positive) constant shifter are on the allow
> list.
> + // Shifters which are above the precision are not well defined, so
> + // that is undefined stmt.
> + case LSHIFT_EXPR:
> + case RSHIFT_EXPR:
> + if (TREE_CODE (gimple_assign_rhs2 (s)) == INTEGER_CST
> + && wi::ltu_p (wi::to_wide (gimple_assign_rhs2 (s)),
> + element_precision (TREE_TYPE (lhs))))
> + break;
> + // Don't allow non-const shifters as they will cause
> + // undefined behavior
> + had_undefined_stmt = true;
> + break;
> + case LROTATE_EXPR:
> + case RROTATE_EXPR:
> + had_undefined_stmt = true;
> + // FALLTHRU
> + default:
> + // Allow all kind of expressions after the first
> + // convert but not with other stmts before hand.
> + if (stmts_minus_convert > 1 && non_allowed_stmt)
> + return invalid_bb;
> + if (has_call)
> + return invalid_bb;
> + non_allowed_stmt = true;
> + break;
> + }
> + }
> +
> + gimple *use_stmt;
> + use_operand_p use_p;
> +
> + /* Allow only a statement which feeds into the next stmt
> + or the phi (when last statement). */
> + if (!lhs || TREE_CODE (lhs) != SSA_NAME
> + || !single_imm_use (lhs, &use_p, &use_stmt)
> + || use_stmt != (gsi_end_p (gsi) ? phi : *gsi ))
> + return invalid_bb;
> + if (!stmt_to_move)
> + stmt_to_move = s;
> }
> - else
> - lhs = gimple_assign_lhs (stmt_to_move);
>
> - gimple *use_stmt;
> - use_operand_p use_p;
> + // CLZ/CTZ is undefined at 0 (or a shift with a non constant)
> + // so if other stmts besides converts moving it will cause
> + // undefined behavior.
> + if (had_undefined_stmt && stmts_minus_convert != 1)
> + return invalid_bb;
>
> - /* Allow only a statement which feeds into the other stmt. */
> - if (!lhs || TREE_CODE (lhs) != SSA_NAME
> - || !single_imm_use (lhs, &use_p, &use_stmt)
> - || use_stmt != phi)
> - return false;
> + // If there are any questionable stmts, make sure there are only
> + // that one (and possible converts).
> + if (stmts_minus_convert != 1 && non_allowed_stmt)
> + return invalid_bb;
>
> + /* The only statement here was a Predict or a nop statement
> + so return 1. */
> + if (stmts == 0)
> + return empty_bb;
> stmt = stmt_to_move;
> - return true;
> + return stmts > 1 ? feeding_bb : one_stmt_bb;
> }
>
> /* Move STMT to before GSI and insert its defining
> @@ -1031,14 +1126,17 @@ move_stmt (gimple *stmt, gimple_stmt_iterator *gsi,
> auto_bitmap &inserted_exprs)
> }
>
> /* RAII style class to temporarily remove flow sensitive
> - from ssa names defined by a gimple statement. */
> + from ssa names defined by a gimple statement or
> + a whole Basic block. */
> class auto_flow_sensitive
> {
> public:
> auto_flow_sensitive (gimple *s);
> + auto_flow_sensitive (basic_block bb);
> ~auto_flow_sensitive ();
> private:
> - auto_vec<std::pair<tree, flow_sensitive_info_storage>, 2> stack;
> + auto_vec<std::pair<tree, flow_sensitive_info_storage>, 4> stack;
> + void save_stmt (gimple *s);
> };
>
> /* Constructor for auto_flow_sensitive. Saves
> @@ -1050,6 +1148,15 @@ auto_flow_sensitive::auto_flow_sensitive (gimple *s)
> {
> if (!s)
> return;
> + save_stmt (s);
> +}
> +
> +/* Saves off the ssa names' flow sensitive information
> + that was defined by gimple statement S and
> + resets it to be non-flow based ones. */
> +void
> +auto_flow_sensitive::save_stmt (gimple *s)
> +{
> ssa_op_iter it;
> tree def;
> FOR_EACH_SSA_TREE_OPERAND (def, s, it, SSA_OP_DEF)
> @@ -1060,6 +1167,21 @@ auto_flow_sensitive::auto_flow_sensitive (gimple *s)
> }
> }
>
> +/* Constructor for auto_flow_sensitive. Saves
> + off the ssa names' flow sensitive information
> + that was defined in the BB and
> + resets it to be non-flow based ones. */
> +
> +auto_flow_sensitive::auto_flow_sensitive (basic_block bb)
> +{
> + if (!bb)
> + return;
> + gimple_stmt_iterator gsi;
> + gsi = gsi_start_nondebug_after_labels_bb (bb);
> + for (; !gsi_end_p (gsi); gsi_next_nondebug (&gsi))
> + save_stmt (*gsi);
> +}
> +
> /* Deconstructor, restores the flow sensitive information
> for the SSA names that had been saved off. */
>
> @@ -1118,14 +1240,20 @@ match_simplify_replacement (basic_block cond_bb,
> basic_block middle_bb,
>
> /* If the basic block only has a cheap preparation statement,
> allow it and move it once the transformation is done. */
> - if (!empty_bb_or_one_feeding_into_p (middle_bb, phi, stmt_to_move))
> + bbfeeding_info middle_bb_info = feeding_bb_info (middle_bb,
> + phi, stmt_to_move);
> + if (middle_bb_info == invalid_bb)
> return false;
>
> + bbfeeding_info middle_bb_alt_info = invalid_bb;
> if (threeway_p
> - && middle_bb != middle_bb_alt
> - && !empty_bb_or_one_feeding_into_p (middle_bb_alt, phi,
> - stmt_to_move_alt))
> - return false;
> + && middle_bb != middle_bb_alt)
> + {
> + middle_bb_alt_info = feeding_bb_info (middle_bb_alt, phi,
> + stmt_to_move_alt);
> + if (middle_bb_alt_info == invalid_bb)
> + return false;
> + }
>
> /* Do not make conditional undefs unconditional. */
> if ((TREE_CODE (arg0) == SSA_NAME
> @@ -1175,12 +1303,15 @@ match_simplify_replacement (basic_block cond_bb,
> basic_block middle_bb,
>
> tree type = TREE_TYPE (gimple_phi_result (phi));
> {
> - auto_flow_sensitive s1(stmt_to_move);
> - auto_flow_sensitive s_alt(stmt_to_move_alt);
> + auto_flow_sensitive s1(middle_bb);
> + basic_block other_bb = nullptr;
> + if (middle_bb_alt_info >= one_stmt_bb)
> + other_bb = middle_bb_alt;
> + auto_flow_sensitive s_alt(other_bb);
>
> result = gimple_simplify_phiopt (early_p, type, stmt,
> arg_true, arg_false,
> - &seq);
> + &seq);
> }
>
> /* For early phiopt, we don't want to lose user generated predictors
> @@ -1220,7 +1351,8 @@ match_simplify_replacement (basic_block cond_bb,
> basic_block middle_bb,
> semantics (that's not IEEE max semantics). */
> if (!HONOR_NANS (type) && !HONOR_SIGNED_ZEROS (type))
> return false;
> - if (stmt_to_move || stmt_to_move_alt)
> + if (middle_bb_info >= one_stmt_bb
> + || middle_bb_alt_info >= one_stmt_bb)
> return false;
> tree_code cmp = gimple_cond_code (stmt);
> if (cmp != LT_EXPR && cmp != LE_EXPR
> @@ -1242,11 +1374,120 @@ match_simplify_replacement (basic_block cond_bb,
> basic_block middle_bb,
> statistics_counter_event (cfun, "Non-IEEE FP MIN/MAX PHI replacement",
> 1);
> }
> - if (dump_file && (dump_flags & TDF_FOLDING))
> - fprintf (dump_file, "accepted the phiopt match-simplify.\n");
>
> auto_bitmap exprs_maybe_dce;
>
> + /* One of the middle BBs had more than one statements. */
> + if (middle_bb_info == feeding_bb
> + || middle_bb_alt_info == feeding_bb)
> + {
> + gimple_stmt_iterator gsi = gsi_start (seq);
> + bool ok = true;
> + walk_stmt_info wi = {};
> + struct op_info {
> + gimple *stmt_to_move;
> + gimple *stmt_to_move_alt;
> + basic_block middle_bb;
> + basic_block middle_bb_alt;
> + bbfeeding_info middle_bb_info;
> + bbfeeding_info middle_bb_alt_info;
> + } info = { stmt_to_move, stmt_to_move_alt,
> + middle_bb, middle_bb_alt,
> + middle_bb_info, middle_bb_alt_info };
> + wi.info = &info;
> + /* Check to see if any of the new sequence uses the definitions
> + from the middle bbs. */
> + for (gsi = gsi_start (seq); !gsi_end_p (gsi); gsi_next (&gsi))
> + {
> + tree use;
> + use = walk_gimple_op (*gsi,
> + [](tree *tp, int *, void *data) -> tree
> + {
> + walk_stmt_info *wi = (walk_stmt_info*)data;
> + op_info *info = (op_info *)wi->info;
> + tree use = *tp;
> + if (!use || TREE_CODE (use) != SSA_NAME)
> + return NULL_TREE;
> + gimple *us = SSA_NAME_DEF_STMT (use);
> + if (!us)
> + return NULL_TREE;
> + if (us == info->stmt_to_move
> + || us == info->stmt_to_move_alt)
> + return NULL_TREE;
> + basic_block use_bb = gimple_bb (us);
> + // If an operand is defined from the middle bb
> + // where there was more one statement, reject it.
> + if ((info->middle_bb_info == feeding_bb
> + && use_bb == info->middle_bb)
> + || (info->middle_bb_alt_info == feeding_bb
> + && use_bb == info->middle_bb_alt))
> + return use;
> + return NULL_TREE;
> + }, &wi);
> + if (use)
> + {
> + ok = false;
So we check whether the middle-BB stmts end up dead? Should we not
simply walk the SSA def chain of 'result', thereby also skipping dead
slack that can end up in seq? We don't have to do it that way, but did
you think about it?
In principle the patch is OK, but it's quite a bit of spaghetti keeping track
of the stmts to move ;) I wish we could do this in fewer lines of code ...
Richard.
> + break;
> + }
> + }
> + if (!ok)
> + {
> + gimple_seq_discard (seq);
> + if (dump_file && (dump_flags & TDF_FOLDING))
> + fprintf (dump_file, "rejected the phiopt match-simplify for
> having more stmts.\n");
> + return false;
> + }
> + statistics_counter_event (cfun, "unconditionalized one middle bb
> phiopt",
> + 1);
> + // If the result is the one of the arguments or a partial
> + // defined in the middle bb, then make those statements unconditional.
> + if (TREE_CODE (result) == SSA_NAME
> + && (middle_bb_info == feeding_bb
> + || middle_bb_alt_info == feeding_bb))
> + {
> + gimple *us = SSA_NAME_DEF_STMT (result);
> + if (us && ((middle_bb_info == feeding_bb
> + && gimple_bb (us) == middle_bb)
> + || (middle_bb_alt_info == feeding_bb
> + && gimple_bb (us) == middle_bb_alt)))
> + {
> + gimple_stmt_iterator middle_gsi;
> + basic_block use_bb = gimple_bb (us);
> + middle_gsi = gsi_start_nondebug_after_labels_bb (use_bb);
> +
> + // Move the statements one at a time until reach the last
> + // statement or the statement that is referenced from the
> result.
> + while (!gsi_end_p (middle_gsi))
> + {
> + // Skip over nop or predicates
> + // Also don't move debug statements
> + if (gimple_code (*middle_gsi) == GIMPLE_NOP
> + || gimple_code (*middle_gsi) == GIMPLE_PREDICT)
> + {
> + gsi_next_nondebug_nonnop_nonpredict (&middle_gsi);
> + continue;
> + }
> + gsi = gsi_last_bb (cond_bb);
> + gimple *s = *middle_gsi;
> + move_stmt (s, &gsi, exprs_maybe_dce);
> +
> + // Reached the last statement that is refenced,
> + // all the rest would be DCEd anyways.
> + if (s == us)
> + break;
> + middle_gsi = gsi_start_nondebug_after_labels_bb (use_bb);
> + }
> + stmt_to_move = nullptr;
> + stmt_to_move_alt = nullptr;
> + statistics_counter_event (cfun, "unconditionalized partial
> middle bb phiopt",
> + 1);
> + }
> + }
> + }
> +
> + if (dump_file && (dump_flags & TDF_FOLDING))
> + fprintf (dump_file, "accepted the phiopt match-simplify.\n");
> +
> /* Mark the cond statements' lhs/rhs as maybe dce. */
> if (TREE_CODE (gimple_cond_lhs (stmt)) == SSA_NAME
> && !SSA_NAME_IS_DEFAULT_DEF (gimple_cond_lhs (stmt)))
> @@ -1272,7 +1513,7 @@ match_simplify_replacement (basic_block cond_bb,
> basic_block middle_bb,
> gsi_insert_seq_before (&gsi, seq, GSI_CONTINUE_LINKING);
> }
>
> - /* If there was a statement to move, move it to right before
> + /* If there was one statement to move, move it to right before
> the original conditional. */
> move_stmt (stmt_to_move, &gsi, exprs_maybe_dce);
> move_stmt (stmt_to_move_alt, &gsi, exprs_maybe_dce);
> --
> 2.43.0
>
