Message ID | ZCaSgegwS47Tq+MJ@tucnak |
---|---|
State | New |
Headers | show |
Series | range-op-float, value-range: Fix up handling of UN{LT,LE,GT,GE,EQ}_EXPR and handle comparisons in get_tree_range [PR91645] | expand |
On 3/31/23 09:57, Jakub Jelinek wrote: > Hi! > > When looking into PR91645, I've noticed we handle UN{LT,LE,GT,GE,EQ}_EXPR > comparisons incorrectly. > All those are unordered or ..., we correctly return [1, 1] if one or > both operands are known NANs, and correctly ask the non-UN prefixed > op to fold_range if neither operand may be NAN. > But for the case where one or both operands may be NAN, we always > return [0, 1]. The UN* fold_range tries to handle it by asking > the non-UN prefixed fold_range and if it returns [1, 1] return that, > if it returns [0, 0] or [0, 1] return [0, 1], which makes sense, > because the maybe NAN means that it is the non-UN prefixed fold_range > unioned with [1, 1] in case the maybe NAN is actually NAN at runtime. > The problem is that the non-UN prefixed fold_range always returns [0, 1] > because those fold_range implementations are like: > if (op1.known_isnan () || op2.known_isnan ()) > r = range_false (type); > else if (!maybe_isnan (op1, op2)) > { > ... > } > else > r = range_true_and_false (type); > and so if maybe_isnan, they always return [0, 1]. Now, thinking about it, > this is unnecessary pessimization, for the case where the ... block > returns range_false (type) we actually could do it also if maybe_isnan (op1, > op2), because if one or both operands are NAN, the comparison will be false, > and if neither is NAN, the comparison will be also false. Will fix > incrementally today. > Anyway, the following patch fixes it by asking the non-UN prefixed > fold_range on ranges with NAN cleared, which I think does the right > thing in all cases. > > Another change in the patch is that range_query::get_tree_range > always returned VARYING for comparisons, this patch allows to ask about > those as well (they are very much like binary ops, except they take > the important type from the types of the operands rather than result). > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? LGTM Aldy > > 2023-03-31 Jakub Jelinek <jakub@redhat.com> > > PR tree-optimization/91645 > * range-op-float.cc (foperator_unordered_lt::fold_range, > foperator_unordered_le::fold_range, > foperator_unordered_gt::fold_range, > foperator_unordered_ge::fold_range, > foperator_unordered_equal::fold_range): Call the ordered > fold_range on ranges with cleared NaNs. > * value-query.cc (range_query::get_tree_range): Handle also > COMPARISON_CLASS_P trees. > > --- gcc/range-op-float.cc.jj 2023-03-28 11:00:29.205986288 +0200 > +++ gcc/range-op-float.cc 2023-03-30 15:14:12.219183066 +0200 > @@ -1587,7 +1587,13 @@ public: > r = range_true (type); > return true; > } > - if (!fop_lt.fold_range (r, type, op1, op2, rel)) > + frange op1_no_nan = op1; > + frange op2_no_nan = op2; > + if (op1.maybe_isnan ()) > + op1_no_nan.clear_nan (); > + if (op2.maybe_isnan ()) > + op2_no_nan.clear_nan (); > + if (!fop_lt.fold_range (r, type, op1_no_nan, op2_no_nan, rel)) > return false; > // The result is the same as the ordered version when the > // comparison is true or when the operands cannot be NANs. > @@ -1692,7 +1698,13 @@ public: > r = range_true (type); > return true; > } > - if (!fop_le.fold_range (r, type, op1, op2, rel)) > + frange op1_no_nan = op1; > + frange op2_no_nan = op2; > + if (op1.maybe_isnan ()) > + op1_no_nan.clear_nan (); > + if (op2.maybe_isnan ()) > + op2_no_nan.clear_nan (); > + if (!fop_le.fold_range (r, type, op1_no_nan, op2_no_nan, rel)) > return false; > // The result is the same as the ordered version when the > // comparison is true or when the operands cannot be NANs. > @@ -1793,7 +1805,13 @@ public: > r = range_true (type); > return true; > } > - if (!fop_gt.fold_range (r, type, op1, op2, rel)) > + frange op1_no_nan = op1; > + frange op2_no_nan = op2; > + if (op1.maybe_isnan ()) > + op1_no_nan.clear_nan (); > + if (op2.maybe_isnan ()) > + op2_no_nan.clear_nan (); > + if (!fop_gt.fold_range (r, type, op1_no_nan, op2_no_nan, rel)) > return false; > // The result is the same as the ordered version when the > // comparison is true or when the operands cannot be NANs. > @@ -1898,7 +1916,13 @@ public: > r = range_true (type); > return true; > } > - if (!fop_ge.fold_range (r, type, op1, op2, rel)) > + frange op1_no_nan = op1; > + frange op2_no_nan = op2; > + if (op1.maybe_isnan ()) > + op1_no_nan.clear_nan (); > + if (op2.maybe_isnan ()) > + op2_no_nan.clear_nan (); > + if (!fop_ge.fold_range (r, type, op1_no_nan, op2_no_nan, rel)) > return false; > // The result is the same as the ordered version when the > // comparison is true or when the operands cannot be NANs. > @@ -2002,7 +2026,13 @@ public: > r = range_true (type); > return true; > } > - if (!fop_equal.fold_range (r, type, op1, op2, rel)) > + frange op1_no_nan = op1; > + frange op2_no_nan = op2; > + if (op1.maybe_isnan ()) > + op1_no_nan.clear_nan (); > + if (op2.maybe_isnan ()) > + op2_no_nan.clear_nan (); > + if (!fop_equal.fold_range (r, type, op1_no_nan, op2_no_nan, rel)) > return false; > // The result is the same as the ordered version when the > // comparison is true or when the operands cannot be NANs. > --- gcc/value-query.cc.jj 2023-03-23 15:25:47.069740988 +0100 > +++ gcc/value-query.cc 2023-03-30 15:20:58.733237311 +0200 > @@ -230,15 +230,21 @@ range_query::get_tree_range (vrange &r, > default: > break; > } > - if (BINARY_CLASS_P (expr)) > + if (BINARY_CLASS_P (expr) || COMPARISON_CLASS_P (expr)) > { > - range_op_handler op (TREE_CODE (expr), type); > + tree op0 = TREE_OPERAND (expr, 0); > + tree op1 = TREE_OPERAND (expr, 1); > + if (COMPARISON_CLASS_P (expr) > + && !Value_Range::supports_type_p (TREE_TYPE (op0))) > + return false; > + range_op_handler op (TREE_CODE (expr), > + BINARY_CLASS_P (expr) ? type : TREE_TYPE (op0)); > if (op) > { > - Value_Range r0 (TREE_TYPE (TREE_OPERAND (expr, 0))); > - Value_Range r1 (TREE_TYPE (TREE_OPERAND (expr, 1))); > - range_of_expr (r0, TREE_OPERAND (expr, 0), stmt); > - range_of_expr (r1, TREE_OPERAND (expr, 1), stmt); > + Value_Range r0 (TREE_TYPE (op0)); > + Value_Range r1 (TREE_TYPE (op1)); > + range_of_expr (r0, op0, stmt); > + range_of_expr (r1, op1, stmt); > if (!op.fold_range (r, type, r0, r1)) > r.set_varying (type); > } > > Jakub >
--- gcc/range-op-float.cc.jj 2023-03-28 11:00:29.205986288 +0200 +++ gcc/range-op-float.cc 2023-03-30 15:14:12.219183066 +0200 @@ -1587,7 +1587,13 @@ public: r = range_true (type); return true; } - if (!fop_lt.fold_range (r, type, op1, op2, rel)) + frange op1_no_nan = op1; + frange op2_no_nan = op2; + if (op1.maybe_isnan ()) + op1_no_nan.clear_nan (); + if (op2.maybe_isnan ()) + op2_no_nan.clear_nan (); + if (!fop_lt.fold_range (r, type, op1_no_nan, op2_no_nan, rel)) return false; // The result is the same as the ordered version when the // comparison is true or when the operands cannot be NANs. @@ -1692,7 +1698,13 @@ public: r = range_true (type); return true; } - if (!fop_le.fold_range (r, type, op1, op2, rel)) + frange op1_no_nan = op1; + frange op2_no_nan = op2; + if (op1.maybe_isnan ()) + op1_no_nan.clear_nan (); + if (op2.maybe_isnan ()) + op2_no_nan.clear_nan (); + if (!fop_le.fold_range (r, type, op1_no_nan, op2_no_nan, rel)) return false; // The result is the same as the ordered version when the // comparison is true or when the operands cannot be NANs. @@ -1793,7 +1805,13 @@ public: r = range_true (type); return true; } - if (!fop_gt.fold_range (r, type, op1, op2, rel)) + frange op1_no_nan = op1; + frange op2_no_nan = op2; + if (op1.maybe_isnan ()) + op1_no_nan.clear_nan (); + if (op2.maybe_isnan ()) + op2_no_nan.clear_nan (); + if (!fop_gt.fold_range (r, type, op1_no_nan, op2_no_nan, rel)) return false; // The result is the same as the ordered version when the // comparison is true or when the operands cannot be NANs. @@ -1898,7 +1916,13 @@ public: r = range_true (type); return true; } - if (!fop_ge.fold_range (r, type, op1, op2, rel)) + frange op1_no_nan = op1; + frange op2_no_nan = op2; + if (op1.maybe_isnan ()) + op1_no_nan.clear_nan (); + if (op2.maybe_isnan ()) + op2_no_nan.clear_nan (); + if (!fop_ge.fold_range (r, type, op1_no_nan, op2_no_nan, rel)) return false; // The result is the same as the ordered version when the // comparison is true or when the operands cannot be NANs. @@ -2002,7 +2026,13 @@ public: r = range_true (type); return true; } - if (!fop_equal.fold_range (r, type, op1, op2, rel)) + frange op1_no_nan = op1; + frange op2_no_nan = op2; + if (op1.maybe_isnan ()) + op1_no_nan.clear_nan (); + if (op2.maybe_isnan ()) + op2_no_nan.clear_nan (); + if (!fop_equal.fold_range (r, type, op1_no_nan, op2_no_nan, rel)) return false; // The result is the same as the ordered version when the // comparison is true or when the operands cannot be NANs. --- gcc/value-query.cc.jj 2023-03-23 15:25:47.069740988 +0100 +++ gcc/value-query.cc 2023-03-30 15:20:58.733237311 +0200 @@ -230,15 +230,21 @@ range_query::get_tree_range (vrange &r, default: break; } - if (BINARY_CLASS_P (expr)) + if (BINARY_CLASS_P (expr) || COMPARISON_CLASS_P (expr)) { - range_op_handler op (TREE_CODE (expr), type); + tree op0 = TREE_OPERAND (expr, 0); + tree op1 = TREE_OPERAND (expr, 1); + if (COMPARISON_CLASS_P (expr) + && !Value_Range::supports_type_p (TREE_TYPE (op0))) + return false; + range_op_handler op (TREE_CODE (expr), + BINARY_CLASS_P (expr) ? type : TREE_TYPE (op0)); if (op) { - Value_Range r0 (TREE_TYPE (TREE_OPERAND (expr, 0))); - Value_Range r1 (TREE_TYPE (TREE_OPERAND (expr, 1))); - range_of_expr (r0, TREE_OPERAND (expr, 0), stmt); - range_of_expr (r1, TREE_OPERAND (expr, 1), stmt); + Value_Range r0 (TREE_TYPE (op0)); + Value_Range r1 (TREE_TYPE (op1)); + range_of_expr (r0, op0, stmt); + range_of_expr (r1, op1, stmt); if (!op.fold_range (r, type, r0, r1)) r.set_varying (type); }