Re: [PATCH] ifcombine: ignore the global ranges of the inner bb [PR126313]

On Wed, Jul 29, 2026 at 4:49 AM Richard Biener
<[email protected]> wrote:
>
> On Sat, Jul 25, 2026 at 6:52 AM Andrea Pinski
> <[email protected]> wrote:
> >
> > ifcombine uses match and match will use in some cases the global
> > range causing wrong code as the range of the ssa name might be based
> > on the outer condition.
> > The case in the bug report is:
> > ```
> >
> >   # RANGE [irange] int [0, 255] MASK 0xff VALUE 0x0
> >   _2 = (int) a.0_1;
> >   if (_2 > 1)
> >     goto <bb 4>; [59.00%]
> >   else
> >     goto <bb 3>; [41.00%]
> >
> >   <bb 3> [local count: 440234144]:
> >   # RANGE [irange] int [0, 1] MASK 0x1 VALUE 0x0
> >   _8 = (int) a.0_1;
> >   if (_2 > _8)
> >     goto <bb 4>; [50.00%]
> >   else
> >     goto <bb 5>; [50.00%]
> > ```
> > So this was `(_2 <= 1 && _2 <= _8) ? goto 5 else; goto 4;`
> > This starts by combnining `_2 <= 1 && _2 <= _8` into `_2 <= min(1, _8)`.
> > But since _8 has a range of [0,1], match invokes the pattern that was added
> > in r14-868-gb06cfb62229f to giving `_2 <= (_8 & 1)` and then since _8 has a
> > range of [0,1], that expression simpifies into `_2 < _8` which is wrong.
> > as _2 is the same as _8. So we end up with not taking the condition any more.
> >
> > The fix is to save off the ranges in the defitions in the inner basic block.
> > phiopt already has code that does this for a statement, this extends it for
> > the whole basic block and moves that class to tree-ssanames.{cc,h}.
> >
> > Bootstrapped and tested on x86_64-linux-gnu with no regressions.
>
> So ISTR I added the 'outer_cond_bb' arguments to maybe_fold_and_comparison
> and friends for a similar issue and a sub-part of that,
> maybe_fold_comparisons_from_match_pd
> already saves off flow-sensitive info.  It seems we're now doing
> duplicate work here?
> Which path in particular is looking at the range and why do the
> existing defenses not work?

So in this case follow_outer_ssa_edges is never called because the
original statements were:
_t = _2 <= 1;
_t1 = _2 <= _8;
This is the temporaries on the stack that is created by
maybe_fold_comparisons_from_match_pd.
gimple match_op is _t & _t1;
So match creates `_2 <= min(1, _8)`
But `min(1, _8)` is simplified first.
Here gimple match_op is min(1, _8) and that matches:
/* Max<bool0, bool1> -> bool0 | bool1
   Min<bool0, bool1> -> bool0 & bool1 */
(for op    (max     min)
     logic (bit_ior bit_and)
 (simplify
  (op zero_one_valued_p@0 zero_one_valued_p@1)
  (logic @0 @1)))

zero_one_valued_p uses tree_nonzero_bits directly which then uses the
global range.
follow_outer_ssa_edges is not called when matching zero_one_valued_p
because it is leaf already (a SSA_NAME).
After you mentioned follow_outer_ssa_edges I did remember this code
but I see it does not handle this case correctly.

So maybe the fix is to call follow_outer_ssa_edges on the inner
operands of the condition that we are building in
maybe_fold_comparisons_from_match_pd (if outer_cond_bb is non-null).
And if the result is null then also return null.
Let me try that.

Thanks,
Andrea


>
> >         PR tree-optimization/126313
> >
> > gcc/ChangeLog:
> >
> >         * tree-ssa-ifcombine.cc (ifcombine_ifandif): Save off the gloabal
> >         ranges of the inner conditional bb.
> >         * tree-ssa-phiopt.cc (class auto_flow_sensitive): Move to
> >         tree-ssanames.h.
> >         (auto_flow_sensitive::auto_flow_sensitive): Move to tree-ssanames.cc.
> >         (auto_flow_sensitive::~auto_flow_sensitive): Likewise.
> >         * tree-ssanames.cc (flow_sensitive_info_storage::need_to_save): New
> >         static method.
> >         (auto_flow_sensitive::auto_flow_sensitive): Moved from
> >         tree-ssa-phiopt.cc and split out into ...
> >         (auto_flow_sensitive::save_stmt): This new method. Add suport for phis.
> >         (auto_flow_sensitive::auto_flow_sensitive): New constructor for basic
> >         block.
> >         (auto_flow_sensitive::~auto_flow_sensitive): Moved from
> >         tree-ssa-phiopt.cc.
> >         * tree-ssanames.h (class auto_flow_sensitive): Moved from
> >         tree-ssa-phopt.cc.
> >
> > gcc/testsuite/ChangeLog:
> >
> >         * gcc.dg/torture/pr126313.c: New test.
> >
> > Signed-off-by: Andrea Pinski <[email protected]>
> > ---
> >  gcc/testsuite/gcc.dg/torture/pr126313.c |  27 ++++++
> >  gcc/tree-ssa-ifcombine.cc               | 110 +++++++++++++-----------
> >  gcc/tree-ssa-phiopt.cc                  |  39 ---------
> >  gcc/tree-ssanames.cc                    |  75 ++++++++++++++++
> >  gcc/tree-ssanames.h                     |  15 ++++
> >  5 files changed, 175 insertions(+), 91 deletions(-)
> >  create mode 100644 gcc/testsuite/gcc.dg/torture/pr126313.c
> >
> > diff --git a/gcc/testsuite/gcc.dg/torture/pr126313.c b/gcc/testsuite/gcc.dg/torture/pr126313.c
> > new file mode 100644
> > index 00000000000..f4b754775fd
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/torture/pr126313.c
> > @@ -0,0 +1,27 @@
> > +/* { dg-do run } */
> > +/* PR tree-optimization/126313 */
> > +unsigned char a = 3;
> > +short b, c, d;
> > +int e;
> > +static inline
> > +char(f)(signed char g, signed char p2) {
> > +  return p2 == 0 || g && p2 == 1 ? 0 : g % p2;
> > +}
> > +static inline
> > +unsigned i(short g) {
> > +  d = g;
> > +  return c;
> > +}
> > +static inline
> > +void fn3(signed char g, int p2) {
> > +  if (!(1 >= p2 && p2 <= g))
> > +    e = b | i(f(1, p2) > 0xE151060F);
> > +  else {
> > +    { d = p2; }
> > +  }
> > +}
> > +int main() {
> > +  fn3(a, a);
> > +  if (d != 0)
> > +    __builtin_abort ();
> > +}
> > diff --git a/gcc/tree-ssa-ifcombine.cc b/gcc/tree-ssa-ifcombine.cc
> > index 0c3e78ef331..1561f2c8b31 100644
> > --- a/gcc/tree-ssa-ifcombine.cc
> > +++ b/gcc/tree-ssa-ifcombine.cc
> > @@ -1000,58 +1000,64 @@ ifcombine_ifandif (basic_block inner_cond_bb, bool inner_inv,
> >           HONOR_NANS (gimple_cond_lhs (outer_cond)));
> >        if (outer_cond_code == ERROR_MARK)
> >         return false;
> > -      /* Don't return false so fast, try maybe_fold_or_comparisons?  */
> > -
> > -      if (!(t = maybe_fold_and_comparisons (boolean_type_node, inner_cond_code,
> > -                                           gimple_cond_lhs (inner_cond),
> > -                                           gimple_cond_rhs (inner_cond),
> > -                                           outer_cond_code,
> > -                                           gimple_cond_lhs (outer_cond),
> > -                                           gimple_cond_rhs (outer_cond),
> > -                                           gimple_bb (outer_cond)))
> > -         && !(t = (fold_truth_andor_for_ifcombine
> > -                   (TRUTH_ANDIF_EXPR, boolean_type_node,
> > -                    gimple_location (outer_cond),
> > -                    outer_cond_code,
> > -                    gimple_cond_lhs (outer_cond),
> > -                    gimple_cond_rhs (outer_cond),
> > -                    gimple_location (inner_cond),
> > -                    inner_cond_code,
> > -                    gimple_cond_lhs (inner_cond),
> > -                    gimple_cond_rhs (inner_cond),
> > -                    single_pred (inner_cond_bb) != outer_cond_bb
> > -                    ? &ts : 0))))
> > -       {
> > -         /* Only combine conditions in this fallback case if the blocks are
> > -            neighbors.  */
> > -         if (single_pred (inner_cond_bb) != outer_cond_bb)
> > -           return false;
> > -         tree t1, t2;
> > -         bool logical_op_non_short_circuit = LOGICAL_OP_NON_SHORT_CIRCUIT;
> > -         if (param_logical_op_non_short_circuit != -1)
> > -           logical_op_non_short_circuit
> > -             = param_logical_op_non_short_circuit;
> > -         if (!logical_op_non_short_circuit || sanitize_coverage_p ())
> > -           return false;
> > -         /* Only do this optimization if the inner bb contains only the conditional
> > -            or there is one or 2 statements which are nop conversion for the comparison. */
> > -         if (!can_combine_bbs_with_short_circuit (inner_cond_bb,
> > -                                                  gimple_cond_lhs (inner_cond),
> > -                                                  gimple_cond_rhs (inner_cond)))
> > -           return false;
> > -         t1 = fold_build2_loc (gimple_location (inner_cond),
> > -                               inner_cond_code,
> > -                               boolean_type_node,
> > -                               gimple_cond_lhs (inner_cond),
> > -                               gimple_cond_rhs (inner_cond));
> > -         t2 = fold_build2_loc (gimple_location (outer_cond),
> > -                               outer_cond_code,
> > -                               boolean_type_node,
> > -                               gimple_cond_lhs (outer_cond),
> > -                               gimple_cond_rhs (outer_cond));
> > -         t = fold_build2_loc (gimple_location (inner_cond),
> > -                              TRUTH_AND_EXPR, boolean_type_node, t1, t2);
> > -        }
> > +
> > +      /* Save off the flow sensitive info for the inner basic block.  */
> > +      {
> > +       auto_flow_sensitive inner_save (inner_cond_bb);
> > +
> > +       /* Don't return false so fast, try maybe_fold_or_comparisons?  */
> > +
> > +       if (!(t = maybe_fold_and_comparisons (boolean_type_node, inner_cond_code,
> > +                                             gimple_cond_lhs (inner_cond),
> > +                                             gimple_cond_rhs (inner_cond),
> > +                                             outer_cond_code,
> > +                                             gimple_cond_lhs (outer_cond),
> > +                                             gimple_cond_rhs (outer_cond),
> > +                                             gimple_bb (outer_cond)))
> > +           && !(t = (fold_truth_andor_for_ifcombine
> > +                     (TRUTH_ANDIF_EXPR, boolean_type_node,
> > +                      gimple_location (outer_cond),
> > +                      outer_cond_code,
> > +                      gimple_cond_lhs (outer_cond),
> > +                      gimple_cond_rhs (outer_cond),
> > +                      gimple_location (inner_cond),
> > +                      inner_cond_code,
> > +                      gimple_cond_lhs (inner_cond),
> > +                      gimple_cond_rhs (inner_cond),
> > +                      single_pred (inner_cond_bb) != outer_cond_bb
> > +                      ? &ts : 0))))
> > +         {
> > +           /* Only combine conditions in this fallback case if the blocks are
> > +              neighbors.  */
> > +           if (single_pred (inner_cond_bb) != outer_cond_bb)
> > +             return false;
> > +           tree t1, t2;
> > +           bool logical_op_non_short_circuit = LOGICAL_OP_NON_SHORT_CIRCUIT;
> > +           if (param_logical_op_non_short_circuit != -1)
> > +             logical_op_non_short_circuit
> > +               = param_logical_op_non_short_circuit;
> > +           if (!logical_op_non_short_circuit || sanitize_coverage_p ())
> > +             return false;
> > +           /* Only do this optimization if the inner bb contains only the conditional
> > +              or there is one or 2 statements which are nop conversion for the comparison. */
> > +           if (!can_combine_bbs_with_short_circuit (inner_cond_bb,
> > +                                                    gimple_cond_lhs (inner_cond),
> > +                                                    gimple_cond_rhs (inner_cond)))
> > +             return false;
> > +           t1 = fold_build2_loc (gimple_location (inner_cond),
> > +                                 inner_cond_code,
> > +                                 boolean_type_node,
> > +                                 gimple_cond_lhs (inner_cond),
> > +                                 gimple_cond_rhs (inner_cond));
> > +           t2 = fold_build2_loc (gimple_location (outer_cond),
> > +                                 outer_cond_code,
> > +                                 boolean_type_node,
> > +                                 gimple_cond_lhs (outer_cond),
> > +                                 gimple_cond_rhs (outer_cond));
> > +           t = fold_build2_loc (gimple_location (inner_cond),
> > +                                TRUTH_AND_EXPR, boolean_type_node, t1, t2);
> > +          }
> > +      }
> >
> >        if (!ifcombine_replace_cond (inner_cond, inner_inv,
> >                                    outer_cond, outer_inv,
> > diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
> > index a5f469e883e..4a1c0660ff3 100644
> > --- a/gcc/tree-ssa-phiopt.cc
> > +++ b/gcc/tree-ssa-phiopt.cc
> > @@ -947,45 +947,6 @@ move_stmt (gimple *stmt, gimple_stmt_iterator *gsi, auto_bitmap &inserted_exprs)
> >      rewrite_to_defined_unconditional (gsi);
> >  }
> >
> > -/* RAII style class to temporarily remove flow sensitive
> > -   from ssa names defined by a gimple statement.  */
> > -class auto_flow_sensitive
> > -{
> > -public:
> > -  auto_flow_sensitive (gimple *s);
> > -  ~auto_flow_sensitive ();
> > -private:
> > -  auto_vec<std::pair<tree, flow_sensitive_info_storage>, 2> stack;
> > -};
> > -
> > -/* Constructor for auto_flow_sensitive. Saves
> > -   off the ssa names' flow sensitive information
> > -   that was defined by gimple statement S and
> > -   resets it to be non-flow based ones. */
> > -
> > -auto_flow_sensitive::auto_flow_sensitive (gimple *s)
> > -{
> > -  if (!s)
> > -    return;
> > -  ssa_op_iter it;
> > -  tree def;
> > -  FOR_EACH_SSA_TREE_OPERAND (def, s, it, SSA_OP_DEF)
> > -    {
> > -      flow_sensitive_info_storage storage;
> > -      storage.save_and_clear (def);
> > -      stack.safe_push (std::make_pair (def, storage));
> > -    }
> > -}
> > -
> > -/* Deconstructor, restores the flow sensitive information
> > -   for the SSA names that had been saved off.  */
> > -
> > -auto_flow_sensitive::~auto_flow_sensitive ()
> > -{
> > -  for (auto p : stack)
> > -    p.second.restore (p.first);
> > -}
> > -
> >  /* Returns true if BB contains an user provided predictor
> >     (PRED_HOT_LABEL/PRED_COLD_LABEL).  */
> >
> > diff --git a/gcc/tree-ssanames.cc b/gcc/tree-ssanames.cc
> > index d2ebb49909f..a451830e1ed 100644
> > --- a/gcc/tree-ssanames.cc
> > +++ b/gcc/tree-ssanames.cc
> > @@ -1060,6 +1060,14 @@ make_pass_release_ssa_names (gcc::context *ctxt)
> >
> >  /* Save and restore of flow sensitive information. */
> >
> > +bool
> > +flow_sensitive_info_storage::need_to_save (tree name)
> > +{
> > +  if (!POINTER_TYPE_P (TREE_TYPE (name)))
> > +    return SSA_NAME_RANGE_INFO (name) != nullptr;
> > +  return SSA_NAME_PTR_INFO (name) != nullptr;
> > +}
> > +
> >  /* Save off the flow sensitive info from NAME. */
> >
> >  void
> > @@ -1129,3 +1137,70 @@ flow_sensitive_info_storage::clear_storage (void)
> >  {
> >    state = 0;
> >  }
> > +
> > +/* Constructor for auto_flow_sensitive. Saves
> > +   off the ssa names' flow sensitive information
> > +   that was defined by gimple statement S and
> > +   resets it to be non-flow based ones. */
> > +
> > +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;
> > +  if (is_a<gphi*> (s))
> > +    {
> > +      def = gimple_phi_result (s);
> > +      if (!flow_sensitive_info_storage::need_to_save (def))
> > +       return;
> > +      flow_sensitive_info_storage storage;
> > +      storage.save_and_clear (def);
> > +      stack.safe_push (std::make_pair (def, storage));
> > +      return;
> > +    }
> > +  FOR_EACH_SSA_TREE_OPERAND (def, s, it, SSA_OP_DEF)
> > +    {
> > +      if (!flow_sensitive_info_storage::need_to_save (def))
> > +       continue;
> > +      flow_sensitive_info_storage storage;
> > +      storage.save_and_clear (def);
> > +      stack.safe_push (std::make_pair (def, storage));
> > +    }
> > +}
> > +
> > +/* 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_phis (bb);
> > +  for (; !gsi_end_p (gsi); gsi_next (&gsi))
> > +    save_stmt (*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.  */
> > +
> > +auto_flow_sensitive::~auto_flow_sensitive ()
> > +{
> > +  for (auto p : stack)
> > +    p.second.restore (p.first);
> > +}
> > diff --git a/gcc/tree-ssanames.h b/gcc/tree-ssanames.h
> > index a7b564dd4e9..4f153d37966 100644
> > --- a/gcc/tree-ssanames.h
> > +++ b/gcc/tree-ssanames.h
> > @@ -147,6 +147,7 @@ public:
> >    void save_and_clear (tree);
> >    void restore (tree);
> >    void clear_storage ();
> > +  static bool need_to_save(tree);
> >  private:
> >    /* 0 means there is nothing saved.
> >       1 means non pointer is saved.
> > @@ -161,4 +162,18 @@ private:
> >    bool null = true;
> >  };
> >
> > +/* RAII style class to temporarily remove flow sensitive
> > +   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>, 4> stack;
> > +  void save_stmt (gimple *s);
> > +};
> > +
> >  #endif /* GCC_TREE_SSANAMES_H */
> > --
> > 2.43.0
> >