Hi Andrea, I’m sending out a respin shortly. Replies inline...
> On 8 Jul 2026, at 08:43, Andrea Pinski <[email protected]> wrote: > > 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. The structural check no longer iterates over the sequence. Each accepted shape is validated along the use-def chain from the final result, so unused statements are not visited. > 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. Done. > >> * 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. Added as predict-15a.c. It keeps the goto predictor exercised without disabling phiopt1. > >> >> 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. Done. phiopt_early_allow now handles exactly: * CMP, with pre-existing operands * -CMP and -(convert)CMP * (convert)CMP, (convert)-CMP and (convert)-(convert)CMP phiopt_skip_mask_conversion is the shared optional-conversion helper and phiopt_self_contained_cmp_p validates the comparison root. > >> + } >> 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 ... I can look at it for a follow-up, for now I was trying to handle just the mask forms... > >> } >> } >> >> @@ -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? Done. The predictor check classifies only the result-defining final statement. > > >> + /* 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? The old gate was too broad. The relevant distinction is not just the mask size, but whether the residual middle remains movable. Materializing a mask in a diamond whose join or arm is entered by another path leaves a residual sub-diamond. If the generated sequence has multiple statements, or the original condition block has a PHI or a preparation statement, that residual middle cannot be moved by late phiopt. ifcombine then loses the original nested branches from which it can form a conditional-compare chain. A bare comparison from a condition-only block remains allowed. Your example is covered by pr125672-4.c and folds completely. The complementary existing ifcombine-ccmp-1.C test has an integer conversion in its condition block. It retains its conditional-compare form. Thanks, Kyrill > 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)
