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

On Fri, Jul 31, 2026 at 4:54 PM Andrea Pinski
<[email protected]> wrote:
>
> 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.

Much simpler change is needed. The problem is follow_outer_ssa_edges
returns NULL in some cases which is fine except it does not remove the
global ranger in those cases.
So it is just a matter of switching around the save and the check for
undefined overflow cases instead.
Will provide a patch in  a few hours once it finishes testing.

Thanks,
Andrea

>
> 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
> > >