public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Jakub Jelinek <jakub@redhat.com>
To: Aldy Hernandez <aldyh@redhat.com>, Andrew MacLeod <amacleod@redhat.com>
Cc: gcc-patches@gcc.gnu.org
Subject: [PATCH] range-op-float, value-range: Fix up handling of UN{LT,LE,GT,GE,EQ}_EXPR and handle comparisons in get_tree_range [PR91645]
Date: Fri, 31 Mar 2023 09:57:54 +0200	[thread overview]
Message-ID: <ZCaSgegwS47Tq+MJ@tucnak> (raw)

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?

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


             reply	other threads:[~2023-03-31 10:01 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-03-31  7:57 Jakub Jelinek [this message]
2023-03-31 10:45 ` [PATCH] range-op-float: Further comparison fixes Jakub Jelinek
2023-03-31 10:57   ` [PATCH] range-op-float: Further foperator_{,not_}equal::fold_range fix Jakub Jelinek
2023-03-31 11:28     ` Aldy Hernandez
2023-03-31 11:28   ` [PATCH] range-op-float: Further comparison fixes Aldy Hernandez
2023-04-01  7:39     ` Jakub Jelinek
2023-04-01 18:32       ` Aldy Hernandez
2023-03-31 11:28 ` [PATCH] range-op-float, value-range: Fix up handling of UN{LT,LE,GT,GE,EQ}_EXPR and handle comparisons in get_tree_range [PR91645] Aldy Hernandez

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ZCaSgegwS47Tq+MJ@tucnak \
    --to=jakub@redhat.com \
    --cc=aldyh@redhat.com \
    --cc=amacleod@redhat.com \
    --cc=gcc-patches@gcc.gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).