On Tue, Jul 7, 2026 at 11:05 PM <[email protected]> wrote: > > From: Kyrylo Tkachov <[email protected]> > > A comparison mask m = cmp ? CST1 : CST2 feeding several tests > if (m == C) is kept as a constant-pair PHI, so each test is a separate > diamond and the backward jump threader tail-duplicates the chain; the > selects then come out as branches instead of conditional moves > (PR125672). > > Let phiopt_early_allow accept the CMP, (convert)(compare) and > -(convert)(compare) forms: sequences consisting only of conversions, > negations and comparisons. An equality test of the materialized mask > folds to the comparison itself, exposing the shared condition for the > pass_merge_diamonds change that follows. > > match_simplify_replacement gains two checks for the new forms: > > * the user predictor check now also covers the multi-statement mask > sequences; > > * a mask is not materialized for a diamond that other paths enter, > through its join or through a shared arm. That would leave a PHI > mixing the mask with the other paths' values, which nothing > downstream recovers, and lose the ccmp chains tree-ifcombine forms > from the original branches. > > Testsuite: pr21090.c, pr68198.c, predict-15.c and pr109071_6.c keep > testing their original shapes with phiopt1 disabled, and new > pr21090-2.c covers the improved folding; vrp102.c scans evrp, where > the tested property is now visible; auto-init-uninit-15.c now warns at > the real use instead of the inlined location, so its previously > xfailed alternative becomes the expectation. > > Run-time neutral on 731.astcenc_r by itself; it enables the 20% > improvement delivered by the following patch. > > Bootstrapped and regression tested on aarch64-unknown-linux-gnu.
I have a conflicting patch which fixes up part of the problem with comparisons that shows up. That is match will sometimes place statements in the sequence which are no longer used. A few comments about this patch too: > > gcc/ChangeLog: > > PR tree-optimization/125672 > PR tree-optimization/126043 > * tree-ssa-phiopt.cc (phiopt_early_allow): Accept CMP, > (convert)(compare) and -(convert)(compare) sequences consisting > only of conversions, negations and comparisons. > (match_simplify_replacement): Extend the predictor keeping check > to the new mask forms. Reject materializing a mask for a diamond > whose join has additional predecessors or whose arm is shared > with another path. > > gcc/testsuite/ChangeLog: > > PR tree-optimization/125672 > PR tree-optimization/126043 > * gcc.dg/tree-ssa/pr125672-1.c: New test. > * gcc.dg/tree-ssa/pr21090-2.c: New test. > * gcc.dg/tree-ssa/pr21090.c: Disable phiopt1. > * gcc.dg/tree-ssa/pr68198.c: Likewise. > * gcc.dg/predict-15.c: Likewise. > * gcc.dg/pr109071_6.c: Likewise. > * gcc.dg/tree-ssa/vrp102.c: Scan evrp instead of vrp1. Please submit gcc.dg/tree-ssa/vrp102.c change separately. > * gcc.dg/auto-init-uninit-15.c: Expect the warning at the use in > bar instead of the inlined location. > > Signed-off-by: Kyrylo Tkachov <[email protected]> > --- > gcc/testsuite/gcc.dg/auto-init-uninit-15.c | 12 ++-- > gcc/testsuite/gcc.dg/pr109071_6.c | 5 +- > gcc/testsuite/gcc.dg/predict-15.c | 4 +- > gcc/testsuite/gcc.dg/tree-ssa/pr125672-1.c | 42 ++++++++++++ > gcc/testsuite/gcc.dg/tree-ssa/pr21090-2.c | 27 ++++++++ > gcc/testsuite/gcc.dg/tree-ssa/pr21090.c | 5 +- > gcc/testsuite/gcc.dg/tree-ssa/pr68198.c | 4 +- > gcc/testsuite/gcc.dg/tree-ssa/vrp102.c | 4 +- > gcc/tree-ssa-phiopt.cc | 80 ++++++++++++++++++---- > 9 files changed, 158 insertions(+), 25 deletions(-) > create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr125672-1.c > create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr21090-2.c > > diff --git a/gcc/testsuite/gcc.dg/auto-init-uninit-15.c > b/gcc/testsuite/gcc.dg/auto-init-uninit-15.c > index 121f0cff274..8d5fdc233b3 100644 > --- a/gcc/testsuite/gcc.dg/auto-init-uninit-15.c > +++ b/gcc/testsuite/gcc.dg/auto-init-uninit-15.c > @@ -1,16 +1,14 @@ > /* PR tree-optimization/17506 > - We issue an uninitialized variable warning at a wrong location at > - line 11, which is very confusing. Make sure we print out a note to > - make it less confusing. (not xfailed alternative) > - But it is of course ok if we warn in bar about uninitialized use > - of j. (not xfailed alternative) */ > + We used to issue an uninitialized variable warning at a wrong (inlined) > + location, which was very confusing. Since early phiopt materializes > + foo's comparison the warning is issued at the real use in bar, with the > note pointing at the declaration. */ > /* { dg-do compile } */ > /* { dg-options "-O1 -Wuninitialized -ftrivial-auto-var-init=zero" } */ > > inline int > foo (int i) > { > - if (i) /* { dg-warning "used uninitialized" } */ > + if (i) > return 1; > return 0; > } > @@ -21,6 +19,6 @@ void > bar (void) > { > int j; /* { dg-message "note: 'j' was declared here" "" } */ > - for (; foo (j); ++j) /* { dg-warning "'j' is used uninitialized" "" { > xfail *-*-* } } */ > + for (; foo (j); ++j) /* { dg-warning "'j' is used uninitialized" } */ > baz (); > } > diff --git a/gcc/testsuite/gcc.dg/pr109071_6.c > b/gcc/testsuite/gcc.dg/pr109071_6.c > index eddf15b350c..6062882583f 100644 > --- a/gcc/testsuite/gcc.dg/pr109071_6.c > +++ b/gcc/testsuite/gcc.dg/pr109071_6.c > @@ -1,7 +1,10 @@ > /* PR tree-optimization/109071 need more context for -Warray-bounds warnings > due to code duplication from jump threading. > test case is from PR117179, which is a duplication of PR109071. */ > -/* { dg-options "-O2 -Warray-bounds -fdiagnostics-show-context=1" } */ > +/* { dg-options "-O2 -Warray-bounds -fdiagnostics-show-context=1 > -fdisable-tree-phiopt1" } */ > +/* phiopt1 is disabled: early phiopt now materializes setval_internal's > + comparison, so no threaded path with the out-of-bounds range reaches > + the array access and no warning is issued at all. */ > /* { dg-additional-options "-fdiagnostics-show-line-numbers > -fdiagnostics-path-format=inline-events -fdiagnostics-show-caret" } */ > /* { dg-enable-nn-line-numbers "" } */ > const char* commands[] = {"a", "b"}; > diff --git a/gcc/testsuite/gcc.dg/predict-15.c > b/gcc/testsuite/gcc.dg/predict-15.c > index 2a8c3ea8597..9a25537c9b6 100644 > --- a/gcc/testsuite/gcc.dg/predict-15.c > +++ b/gcc/testsuite/gcc.dg/predict-15.c > @@ -1,5 +1,7 @@ > /* { dg-do compile } */ > -/* { dg-options "-O2 -fdump-tree-profile_estimate" } */ > +/* { dg-options "-O2 -fdump-tree-profile_estimate -fdisable-tree-phiopt1" } > */ > +/* phiopt1 is disabled: early phiopt now materializes the comparison > + and no goto edge remains to predict. */ I think we should add another testcase which adds a function call before both returns. > > int main(int argc, char **argv) > { > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr125672-1.c > b/gcc/testsuite/gcc.dg/tree-ssa/pr125672-1.c > new file mode 100644 > index 00000000000..6cc90f656c5 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr125672-1.c > @@ -0,0 +1,42 @@ > +/* Early phiopt materializes a comparison mask "cmp ? CST1 : CST2" as > + -(int)cmp / (int)cmp, exposing its comparison. An equality test of the > + mask ("m == C") then folds to the comparison itself, so the mask > + disappears and diamonds controlled by the same mask share an identical > + condition (a prerequisite for pass_merge_diamonds). */ > +/* { dg-do compile } */ > +/* { dg-options "-O2 -fdump-tree-phiopt1" } */ > + > +/* -(int)cmp all-ones mask tested against a constant: the test folds to the > + comparison. */ > +int > +neg_masktest (int a, int b, int x, int y) > +{ > + int m; > + if (a > b) m = -1; else m = 0; > + return (m == -1) ? x : y; > +} > + > +/* (int)cmp boolean mask tested against a constant (the convert case). */ > +int > +cvt_masktest (int a, int b, int x, int y) > +{ > + int m; > + if (a > b) m = 1; else m = 0; > + return (m == 1) ? x : y; > +} > + > +/* A mask consumed by other operators is materialized too. */ > +int > +mask_algebra (int a, int b) > +{ > + int m; > + if (a > b) m = -1; else m = 0; > + return m & 7; > +} > + > +/* All three masks are materialized: no constant-pair mask PHI survives and > + the equality tests fold away. */ > +/* { dg-final { scan-tree-dump-not "= PHI <-1\\(" "phiopt1" } } */ > +/* { dg-final { scan-tree-dump-not "== -1" "phiopt1" } } */ > +/* { dg-final { scan-tree-dump-not "== 1" "phiopt1" } } */ > +/* { dg-final { scan-tree-dump "& 7" "phiopt1" } } */ > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr21090-2.c > b/gcc/testsuite/gcc.dg/tree-ssa/pr21090-2.c > new file mode 100644 > index 00000000000..d3792d4314f > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr21090-2.c > @@ -0,0 +1,27 @@ > +/* With early phiopt lowering "p != 0 ? 1 : 0" to > + a boolean mask, the non-null test of a PHI of two object addresses folds > + away entirely, without needing VRP's predicate folding (compare > + pr21090.c). */ > +/* { dg-do compile } */ > +/* { dg-options "-O2 -fno-thread-jumps -fdelete-null-pointer-checks > -fdump-tree-optimized" } */ > + > +int g, h; > + > +int > +foo (int a) > +{ > + int *p; > + > + if (a) > + p = &g; > + else > + p = &h; > + > + if (p != 0) > + return 1; > + else > + return 0; > +} > + > +/* { dg-final { scan-tree-dump "return 1;" "optimized" { target { ! > keeps_null_pointer_checks } } } } */ > +/* { dg-final { scan-tree-dump-not "PHI" "optimized" { target { ! > keeps_null_pointer_checks } } } } */ > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr21090.c > b/gcc/testsuite/gcc.dg/tree-ssa/pr21090.c > index 92a87688601..ef755e85270 100644 > --- a/gcc/testsuite/gcc.dg/tree-ssa/pr21090.c > +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr21090.c > @@ -1,5 +1,8 @@ > /* { dg-do compile } */ > -/* { dg-options "-O2 -fno-thread-jumps -fdisable-tree-evrp -fdump-tree-vrp1 > -fdelete-null-pointer-checks" } */ > +/* { dg-options "-O2 -fno-thread-jumps -fdisable-tree-evrp -fdump-tree-vrp1 > -fdelete-null-pointer-checks -fdisable-tree-phiopt1" } */ > +/* phiopt1 is disabled so the non-null test reaches VRP as a branch; early > + phiopt now lowers it to a boolean mask that is folded even earlier, > + which pr21090-2.c covers. */ > > int g, h; > > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr68198.c > b/gcc/testsuite/gcc.dg/tree-ssa/pr68198.c > index 125072941da..3b33961268f 100644 > --- a/gcc/testsuite/gcc.dg/tree-ssa/pr68198.c > +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr68198.c > @@ -1,5 +1,7 @@ > /* { dg-do compile } */ > -/* { dg-options "-O2 -fdump-tree-threadfull1-details -fdisable-tree-ethread" > } */ > +/* { dg-options "-O2 -fdump-tree-threadfull1-details -fdisable-tree-ethread > -fdisable-tree-phiopt1" } */ > +/* phiopt1 is disabled: early phiopt now lowers the remove test to a boolean > + mask and these threading opportunities change shape. */ > > extern void abort (void); > > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/vrp102.c > b/gcc/testsuite/gcc.dg/tree-ssa/vrp102.c > index fb62e570bed..83f6d98796f 100644 > --- a/gcc/testsuite/gcc.dg/tree-ssa/vrp102.c > +++ b/gcc/testsuite/gcc.dg/tree-ssa/vrp102.c > @@ -1,5 +1,5 @@ > /* { dg-do compile } */ > -/* { dg-options "-O2 -fno-tree-dominator-opts -fdump-tree-vrp1" } */ > +/* { dg-options "-O2 -fno-tree-dominator-opts -fdump-tree-evrp" } */ > > int f(int x, int y) > { > @@ -15,4 +15,4 @@ int f(int x, int y) > /* We should have computed x ^ y as zero and propagated the result into the > PHI feeding the result. */ > > -/* { dg-final { scan-tree-dump "ret_\[0-9\]+ = PHI <\[01\]\\\(\[0-9\]+\\\), > \[01\]\\\(\[0-9\]+\\\)>" "vrp1" } } */ > +/* { dg-final { scan-tree-dump "ret_\[0-9\]+ = PHI <\[01\]\\\(\[0-9\]+\\\), > \[01\]\\\(\[0-9\]+\\\)>" "evrp" } } */ > diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc > index e12dc7a8b0c..df658b04cfb 100644 > --- a/gcc/tree-ssa-phiopt.cc > +++ b/gcc/tree-ssa-phiopt.cc > @@ -666,6 +666,29 @@ phiopt_early_allow (gimple_seq &seq, gimple_match_op &op) > code = gimple_assign_rhs_code (stmt); > return code == MIN_EXPR || code == MAX_EXPR; > } > + /* Accept CMP, (convert)(compare) and -(convert)(compare) when the > + generated stmts are only conversions and comparisons, so a mask such > as > + "cmp ? -1 : 0" / "cmp ? 1 : 0" can be materialized as -(int)cmp / > + (int)cmp, exposing its comparison. This is only the structural test; > + match_simplify_replacement additionally keeps user predictors alive > + and leaves diamonds that other paths enter alone. */ > + if (code == NEGATE_EXPR > + || CONVERT_EXPR_CODE_P (code) > + || TREE_CODE_CLASS (code) == tcc_comparison) > + { > + for (gimple_stmt_iterator gsi = gsi_start (seq); !gsi_end_p (gsi); > + gsi_next (&gsi)) > + { > + gimple *s = gsi_stmt (gsi); > + if (!is_gimple_assign (s)) > + return false; > + tree_code scode = gimple_assign_rhs_code (s); > + if (!CONVERT_EXPR_CODE_P (scode) > + && TREE_CODE_CLASS (scode) != tcc_comparison) > + return false; > + } > + return true; This is 100% wrong. You want a case for each. Part of the reason why it is wrong is because of the extra statements as I mentioned before. The second reason why it is wrong is because it is accepting way too much. For NEGATE_EXPR, you should only accept a comparison or one convert that is followed by a comparison. For a comparison, you should only accept that and nothing else (at least for now; though that might change in the future). For a convert, this becomes more complex but you want to optionally select a convert followed by a negate expr followed by an optional convert followed by a comparison (note this is in reverse order so gsi_last rather than gsi_begin). You can make a helper function for the optional convert and use it for the NEGATE_EXPR too. > + } > return false; > } > > @@ -684,7 +707,8 @@ phiopt_early_allow (gimple_seq &seq, gimple_match_op &op) > case FIXED_CST: > return true; > default: > - return false; > + /* A bare comparison (CMP) is also allowed. */ > + return TREE_CODE_CLASS (code) == tcc_comparison; We might want to accept a bare convert too ... > } > } > > @@ -1101,9 +1125,9 @@ match_simplify_replacement (basic_block cond_bb, > basic_block middle_bb, > } > > /* For early phiopt, we don't want to lose user generated predictors > - if the phiopt is converting `if (a)` into `a` as that might > - be jump threaded later on so we want to keep around the > - predictors. */ > + if the phiopt is converting `if (a)` into `a` (or into the new > + (convert)cmp / -(convert)cmp mask forms) as that might be jump > + threaded later on so we want to keep around the predictors. */ > if (early_p && result && TREE_CODE (result) == SSA_NAME) > { > bool check_it = false; > @@ -1111,15 +1135,33 @@ match_simplify_replacement (basic_block cond_bb, > basic_block middle_bb, > tree cmp1 = gimple_cond_rhs (stmt); > if (result == cmp0 || result == cmp1) > check_it = true; > - else if (gimple_seq_singleton_p (seq)) > - { > - gimple *stmt = gimple_seq_first_stmt (seq); > - if (is_gimple_assign (stmt) > - && result == gimple_assign_lhs (stmt) > - && TREE_CODE_CLASS (gimple_assign_rhs_code (stmt)) > - == tcc_comparison) > + else if (!gimple_seq_empty_p (seq)) > + { We can just check the last statement instead of the first. No need for a loop. If the last statement is a comparison, convert or a negate expression and set check_it. Your patches always seem to be full of loops why? > + /* A comparison mask: the generated statements are only > + conversions, negations and comparisons, with at least one > + comparison (the forms phiopt_early_allow accepts). */ > + bool has_cmp = false, only_mask_ops = true; > + for (gimple_stmt_iterator gi = gsi_start (seq); !gsi_end_p (gi); > + gsi_next (&gi)) > + { > + gimple *gs = gsi_stmt (gi); > + if (!is_gimple_assign (gs)) > + { > + only_mask_ops = false; > + break; > + } > + tree_code c = gimple_assign_rhs_code (gs); > + if (TREE_CODE_CLASS (c) == tcc_comparison) > + has_cmp = true; > + else if (!CONVERT_EXPR_CODE_P (c) && c != NEGATE_EXPR) > + { > + only_mask_ops = false; > + break; > + } > + } > + if (only_mask_ops && has_cmp) > check_it = true; > - } > + } > if (!check_it) > ; > else if (contains_hot_cold_predict (middle_bb)) > @@ -1128,6 +1170,20 @@ match_simplify_replacement (basic_block cond_bb, > basic_block middle_bb, > && middle_bb != middle_bb_alt > && contains_hot_cold_predict (middle_bb_alt)) > return false; > + /* Materializing a comparison mask for a diamond that other paths > + enter (through its join or through a shared arm) would leave > + a PHI mixing the mask with the other paths' values: a residual > + half-diamond that neither ifcombine nor RTL if-conversion > + recovers, whereas they do handle the original nested branches > + (e.g. into ccmp chains). Leave such embedded diamonds alone. */ > + else if (result != cmp0 && result != cmp1 > + && (EDGE_COUNT (gimple_bb (phi)->preds) != 2 > + || (middle_bb != gimple_bb (phi) > + && !single_pred_p (middle_bb)) > + || (threeway_p > + && middle_bb_alt != gimple_bb (phi) > + && !single_pred_p (middle_bb_alt)))) > + return false; I do not understand this. Or why do you need it? Especially if you have: ``` bool f(int a) { if (a < 10) return 1; if (a < 3) return 1; return 0; } ``` (compile with `-O1 -fno-thread-jumps -fdisable-tree-ifcombine`). Thanks, Andrea > } > > if (!result) > -- > 2.50.1 (Apple Git-155) >
