[COMMITTED,range-op-float] Abstract out binary operator code out of PLUS_EXPR entry.

Message ID 20221109070758.1030615-1-aldyh@redhat.com
State Unresolved
Headers
Series [COMMITTED,range-op-float] Abstract out binary operator code out of PLUS_EXPR entry. |

Checks

Context Check Description
snail/gcc-patch-check warning Git am fail log

Commit Message

Aldy Hernandez Nov. 9, 2022, 7:07 a.m. UTC
  The PLUS_EXPR was always meant to be a template for further
development, since most of the binary operators will share a similar
structure.  This patch abstracts out the common bits into the default
definition for range_operator_float::fold_range() and provides an
rv_fold() to be implemented by the individual entries wishing to use
the generic folder.  This is akin to what we do with fold_range() and
wi_fold() in the integer version of range-ops.

gcc/ChangeLog:

	* range-op-float.cc (range_operator_float::fold_range): Abstract
	out from foperator_plus.
	(range_operator_float::rv_fold): New.
	(foperator_plus::fold_range): Remove.
	(foperator_plus::rv_fold): New.
	(propagate_nans): Remove.
	* range-op.h (class range_operator_float): Add rv_fold.
---
 gcc/range-op-float.cc | 156 +++++++++++++++++++++---------------------
 gcc/range-op.h        |   7 ++
 2 files changed, 84 insertions(+), 79 deletions(-)
  

Comments

Jakub Jelinek Nov. 9, 2022, 12:48 p.m. UTC | #1
On Wed, Nov 09, 2022 at 08:07:57AM +0100, Aldy Hernandez wrote:
> The PLUS_EXPR was always meant to be a template for further
> development, since most of the binary operators will share a similar
> structure.  This patch abstracts out the common bits into the default
> definition for range_operator_float::fold_range() and provides an
> rv_fold() to be implemented by the individual entries wishing to use
> the generic folder.  This is akin to what we do with fold_range() and
> wi_fold() in the integer version of range-ops.

Shouldn't foperator_mult be very similar to this (except that until
division is done op[12]_range can't be implemented), with the exception
that the invalid case isn't -INF + INF or INF + -INF, but
0 * +/-INF or +/-INF * 0?

	Jakub
  
Aldy Hernandez Nov. 9, 2022, 1:14 p.m. UTC | #2
On Wed, Nov 9, 2022 at 1:48 PM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Wed, Nov 09, 2022 at 08:07:57AM +0100, Aldy Hernandez wrote:
> > The PLUS_EXPR was always meant to be a template for further
> > development, since most of the binary operators will share a similar
> > structure.  This patch abstracts out the common bits into the default
> > definition for range_operator_float::fold_range() and provides an
> > rv_fold() to be implemented by the individual entries wishing to use
> > the generic folder.  This is akin to what we do with fold_range() and
> > wi_fold() in the integer version of range-ops.
>
> Shouldn't foperator_mult be very similar to this (except that until
> division is done op[12]_range can't be implemented), with the exception
> that the invalid case isn't -INF + INF or INF + -INF, but
> 0 * +/-INF or +/-INF * 0?

Multiplication and division are tricky because you have to keep track
of signs to order the resulting range.  It's the most annoying pattern
we have for integers:

// Multiplications, divisions and shifts are a bit tricky to handle,
// depending on the mix of signs we have in the two ranges, we need to
// operate on different values to get the minimum and maximum values
// for the new range.  One approach is to figure out all the
// variations of range combinations and do the operations.
//
// However, this involves several calls to compare_values and it is
// pretty convoluted.  It's simpler to do the 4 operations (MIN0 OP
// MIN1, MIN0 OP MAX1, MAX0 OP MIN1 and MAX0 OP MAX0 OP MAX1) and then
// figure the smallest and largest values to form the new range.

But if you have a simpler approach, have at it.  I may have to bail on
multiplication and division for this cycle, cause I'm running out of
cycles :-/.

Hmmm...even if we don't get to implement mult/div in this cycle,
perhaps we could at least figure out if we'll NAN as you've suggested
above.  So, set [-INF,+INF] but without a NAN when applicable.

Aldy
  
Jakub Jelinek Nov. 9, 2022, 1:32 p.m. UTC | #3
On Wed, Nov 09, 2022 at 02:14:19PM +0100, Aldy Hernandez wrote:
> On Wed, Nov 9, 2022 at 1:48 PM Jakub Jelinek <jakub@redhat.com> wrote:
> >
> > On Wed, Nov 09, 2022 at 08:07:57AM +0100, Aldy Hernandez wrote:
> > > The PLUS_EXPR was always meant to be a template for further
> > > development, since most of the binary operators will share a similar
> > > structure.  This patch abstracts out the common bits into the default
> > > definition for range_operator_float::fold_range() and provides an
> > > rv_fold() to be implemented by the individual entries wishing to use
> > > the generic folder.  This is akin to what we do with fold_range() and
> > > wi_fold() in the integer version of range-ops.
> >
> > Shouldn't foperator_mult be very similar to this (except that until
> > division is done op[12]_range can't be implemented), with the exception
> > that the invalid case isn't -INF + INF or INF + -INF, but
> > 0 * +/-INF or +/-INF * 0?
> 
> Multiplication and division are tricky because you have to keep track
> of signs to order the resulting range.  It's the most annoying pattern
> we have for integers:

Ah, you're right.
Reminds me of check_for_binary_op_overflow for multiplication.

	Jakub
  
Jakub Jelinek Nov. 9, 2022, 2:19 p.m. UTC | #4
On Wed, Nov 09, 2022 at 02:32:55PM +0100, Jakub Jelinek wrote:
> On Wed, Nov 09, 2022 at 02:14:19PM +0100, Aldy Hernandez wrote:
> > On Wed, Nov 9, 2022 at 1:48 PM Jakub Jelinek <jakub@redhat.com> wrote:
> > >
> > > On Wed, Nov 09, 2022 at 08:07:57AM +0100, Aldy Hernandez wrote:
> > > > The PLUS_EXPR was always meant to be a template for further
> > > > development, since most of the binary operators will share a similar
> > > > structure.  This patch abstracts out the common bits into the default
> > > > definition for range_operator_float::fold_range() and provides an
> > > > rv_fold() to be implemented by the individual entries wishing to use
> > > > the generic folder.  This is akin to what we do with fold_range() and
> > > > wi_fold() in the integer version of range-ops.
> > >
> > > Shouldn't foperator_mult be very similar to this (except that until
> > > division is done op[12]_range can't be implemented), with the exception
> > > that the invalid case isn't -INF + INF or INF + -INF, but
> > > 0 * +/-INF or +/-INF * 0?
> > 
> > Multiplication and division are tricky because you have to keep track
> > of signs to order the resulting range.  It's the most annoying pattern
> > we have for integers:
> 
> Ah, you're right.
> Reminds me of check_for_binary_op_overflow for multiplication.

On the other side, thinking more about it, it should be easier than
integral, because we don't need to deal with unsigned/wrap around and
overflows aren't undefined, but infinities (though I guess we still have
even for +/- the question how actually say PDP floats or ARM non-IEEE
mode __fp16 behave on overflows for floats that don't support infinities,
if it is saturating on HUGE_VAL*/-HUGE_VAL* or wraps around).
So just do the cross products, sort them to create the final range,
clear_nans on it and provide nans the normal way?

	Jakub
  

Patch

diff --git a/gcc/range-op-float.cc b/gcc/range-op-float.cc
index 8282c912fc4..7075c25442a 100644
--- a/gcc/range-op-float.cc
+++ b/gcc/range-op-float.cc
@@ -49,13 +49,66 @@  along with GCC; see the file COPYING3.  If not see
 // Default definitions for floating point operators.
 
 bool
-range_operator_float::fold_range (frange &r ATTRIBUTE_UNUSED,
-				  tree type ATTRIBUTE_UNUSED,
-				  const frange &lh ATTRIBUTE_UNUSED,
-				  const frange &rh ATTRIBUTE_UNUSED,
+range_operator_float::fold_range (frange &r, tree type,
+				  const frange &op1, const frange &op2,
 				  relation_trio) const
 {
-  return false;
+  if (empty_range_varying (r, type, op1, op2))
+    return true;
+  if (op1.known_isnan () || op2.known_isnan ())
+    {
+      r.set_nan (op1.type ());
+      return true;
+    }
+
+  REAL_VALUE_TYPE lb, ub;
+  bool maybe_nan;
+  rv_fold (lb, ub, maybe_nan, type,
+	   op1.lower_bound (), op1.upper_bound (),
+	   op2.lower_bound (), op2.upper_bound ());
+
+  // Handle possible NANs by saturating to the appropriate INF if only
+  // one end is a NAN.  If both ends are a NAN, just return a NAN.
+  bool lb_nan = real_isnan (&lb);
+  bool ub_nan = real_isnan (&ub);
+  if (lb_nan && ub_nan)
+    {
+      r.set_nan (type);
+      return true;
+    }
+  if (lb_nan)
+    lb = dconstninf;
+  else if (ub_nan)
+    ub = dconstinf;
+
+  r.set (type, lb, ub);
+
+  if (lb_nan || ub_nan || maybe_nan)
+    // Keep the default NAN (with a varying sign) set by the setter.
+    ;
+  else if (!op1.maybe_isnan () && !op2.maybe_isnan ())
+    r.clear_nan ();
+
+  return true;
+}
+
+// For a given operation, fold two sets of ranges into [lb, ub].
+// MAYBE_NAN is set to TRUE if, in addition to any result in LB or
+// UB, the final range has the possiblity of a NAN.
+void
+range_operator_float::rv_fold (REAL_VALUE_TYPE &lb,
+			       REAL_VALUE_TYPE &ub,
+			       bool &maybe_nan,
+			       tree type ATTRIBUTE_UNUSED,
+			       const REAL_VALUE_TYPE &lh_lb ATTRIBUTE_UNUSED,
+			       const REAL_VALUE_TYPE &lh_ub ATTRIBUTE_UNUSED,
+			       const REAL_VALUE_TYPE &rh_lb ATTRIBUTE_UNUSED,
+			       const REAL_VALUE_TYPE &rh_ub ATTRIBUTE_UNUSED)
+  const
+{
+  lb = dconstninf;
+  ub = dconstinf;
+  maybe_nan = true;
 }
 
 bool
@@ -192,19 +245,6 @@  frelop_early_resolve (irange &r, tree type,
 	  && relop_early_resolve (r, type, op1, op2, rel, my_rel));
 }
 
-// If either operand is a NAN, set R to NAN and return TRUE.
-
-inline bool
-propagate_nans (frange &r, const frange &op1, const frange &op2)
-{
-  if (op1.known_isnan () || op2.known_isnan ())
-    {
-      r.set_nan (op1.type ());
-      return true;
-    }
-  return false;
-}
-
 // Set VALUE to its next real value, or INF if the operation overflows.
 
 inline void
@@ -1822,69 +1862,27 @@  foperator_unordered_equal::op1_range (frange &r, tree type,
 
 class foperator_plus : public range_operator_float
 {
-  using range_operator_float::fold_range;
-
-public:
-  bool fold_range (frange &r, tree type,
-		   const frange &lh,
-		   const frange &rh,
-		   relation_trio = TRIO_VARYING) const final override;
+  void rv_fold (REAL_VALUE_TYPE &lb, REAL_VALUE_TYPE &ub, bool &maybe_nan,
+		tree type,
+		const REAL_VALUE_TYPE &lh_lb,
+		const REAL_VALUE_TYPE &lh_ub,
+		const REAL_VALUE_TYPE &rh_lb,
+		const REAL_VALUE_TYPE &rh_ub) const final override
+  {
+    frange_arithmetic (PLUS_EXPR, type, lb, lh_lb, rh_lb, dconstninf);
+    frange_arithmetic (PLUS_EXPR, type, ub, lh_ub, rh_ub, dconstinf);
+
+    // [-INF] + [+INF] = NAN
+    if (real_isinf (&lh_lb, true) && real_isinf (&rh_ub, false))
+      maybe_nan = true;
+    // [+INF] + [-INF] = NAN
+    else if (real_isinf (&lh_ub, false) && real_isinf (&rh_lb, true))
+      maybe_nan = true;
+    else
+      maybe_nan = false;
+  }
 } fop_plus;
 
-bool
-foperator_plus::fold_range (frange &r, tree type,
-			    const frange &op1, const frange &op2,
-			    relation_trio) const
-{
-  if (empty_range_varying (r, type, op1, op2))
-    return true;
-  if (propagate_nans (r, op1, op2))
-    return true;
-
-  REAL_VALUE_TYPE lb, ub;
-  frange_arithmetic (PLUS_EXPR, type, lb,
-		     op1.lower_bound (), op2.lower_bound (), dconstninf);
-  frange_arithmetic (PLUS_EXPR, type, ub,
-		     op1.upper_bound (), op2.upper_bound (), dconstinf);
-
-  // Handle possible NANs by saturating to the appropriate INF if only
-  // one end is a NAN.  If both ends are a NAN, just return a NAN.
-  bool lb_nan = real_isnan (&lb);
-  bool ub_nan = real_isnan (&ub);
-  if (lb_nan && ub_nan)
-    {
-      r.set_nan (type);
-      return true;
-    }
-  if (lb_nan)
-    lb = dconstninf;
-  else if (ub_nan)
-    ub = dconstinf;
-
-  r.set (type, lb, ub);
-
-  // Some combinations can yield a NAN even if no operands have the
-  // possibility of a NAN.
-  bool maybe_nan;
-  // [-INF] + [+INF] = NAN
-  if (real_isinf (&op1.lower_bound (), true)
-      && real_isinf (&op2.upper_bound (), false))
-    maybe_nan = true;
-  // [+INF] + [-INF] = NAN
-  else if (real_isinf (&op1.upper_bound (), false)
-	   && real_isinf (&op2.lower_bound (), true))
-    maybe_nan = true;
-  else
-    maybe_nan = false;
-
-  if (lb_nan || ub_nan || maybe_nan)
-    // Keep the default NAN (with a varying sign) set by the setter.
-    ;
-  else if (!op1.maybe_isnan () && !op2.maybe_isnan ())
-    r.clear_nan ();
-
-  return true;
-}
 
 // Instantiate a range_op_table for floating point operations.
 static floating_op_table global_floating_table;
diff --git a/gcc/range-op.h b/gcc/range-op.h
index c7249890142..442a6e1d299 100644
--- a/gcc/range-op.h
+++ b/gcc/range-op.h
@@ -117,6 +117,13 @@  public:
 			   const frange &lh,
 			   const frange &rh,
 			   relation_trio = TRIO_VARYING) const;
+  virtual void rv_fold (REAL_VALUE_TYPE &lb, REAL_VALUE_TYPE &ub,
+			bool &maybe_nan,
+			tree type,
+			const REAL_VALUE_TYPE &lh_lb,
+			const REAL_VALUE_TYPE &lh_ub,
+			const REAL_VALUE_TYPE &rh_lb,
+			const REAL_VALUE_TYPE &rh_ub) const;
   // Unary operations have the range of the LHS as op2.
   virtual bool fold_range (irange &r, tree type,
 			   const frange &lh,