public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Aldy Hernandez <aldyh@redhat.com>
To: Mikael Morin <morin-mikael@orange.fr>, gcc-patches@gcc.gnu.org
Cc: Andrew MacLeod <amacleod@redhat.com>
Subject: Re: [PATCH] Rewrite NAN and sign handling in frange
Date: Wed, 2 Nov 2022 14:35:41 +0100	[thread overview]
Message-ID: <08ea88c3-df7a-acdd-1a75-b43d81225712@redhat.com> (raw)
In-Reply-To: <46a9380d-2cd5-7bfb-ecc7-a33a8369338b@orange.fr>

[-- Attachment #1: Type: text/plain, Size: 2358 bytes --]



On 9/27/22 15:00, Mikael Morin wrote:
> Hello,
> 
> Le 16/09/2022 à 15:26, Aldy Hernandez via Gcc-patches a écrit :
>> diff --git a/gcc/value-range.cc b/gcc/value-range.cc
>> index d759fcf178c..55a216efd8b 100644
>> --- a/gcc/value-range.cc
>> +++ b/gcc/value-range.cc
>> @@ -617,21 +602,24 @@ frange::contains_p (tree cst) const
>>    if (varying_p ())
>>      return true;
>>
>   (...)
>>
>>    if (real_compare (GE_EXPR, rv, &m_min) && real_compare (LE_EXPR, 
>> rv, &m_max))
>>      {
>> +      // Make sure the signs are equal for signed zeros.
>>        if (HONOR_SIGNED_ZEROS (m_type) && real_iszero (rv))
>> -    {
>> -      // FIXME: This is still using get_signbit() instead of
>> -      // known_signbit() because the latter bails on possible NANs
>> -      // (for now).
>> -      if (get_signbit ().yes_p ())
>> -        return real_isneg (rv);
>> -      else if (get_signbit ().no_p ())
>> -        return !real_isneg (rv);
>> -      else
>> -        return true;
>> -    }
>> +    return m_min.sign == m_max.sign && m_min.sign == rv->sign;
>>        return true;
>>      }
>>    return false;
> 
> It seems that this won't report any range with mismatching bound signs 
> as containing zero.
> Maybe a selftest explains it better: the following fails.

My apologies for only seeing this now.  You did not CC me in the 
response, and it got lost amongst my other list mail.

You are absolutely right.

The attached patch fixes this problem.  It has been tested on x86-64 
Linux and pushed.

Thanks for pointing this out.
Aldy

> 
> diff --git a/gcc/value-range.cc b/gcc/value-range.cc
> index 9ca442478c9..8fc909171bc 100644
> --- a/gcc/value-range.cc
> +++ b/gcc/value-range.cc
> @@ -3780,6 +3780,14 @@ range_tests_signed_zeros ()
>     ASSERT_TRUE (r0.contains_p (neg_zero));
>     ASSERT_FALSE (r0.contains_p (zero));
> 
> +  r0 = frange_float ("-3", "5");
> +  ASSERT_TRUE (r0.contains_p (neg_zero));
> +  ASSERT_TRUE (r0.contains_p (zero));
> +
> +  r0 = frange (neg_zero, zero);
> +  ASSERT_TRUE (r0.contains_p (neg_zero));
> +  ASSERT_TRUE (r0.contains_p (zero));
> +
>     // The intersection of zeros that differ in sign is a NAN (or
>     // undefined if not honoring NANs).
>     r0 = frange (neg_zero, neg_zero);
> 

[-- Attachment #2: 0001-Fix-bug-in-frange-contains_p-for-signed-zeros.patch --]
[-- Type: text/x-patch, Size: 1618 bytes --]

From da2d128a87b2f3359f6f38f29624387094875a60 Mon Sep 17 00:00:00 2001
From: Aldy Hernandez <aldyh@redhat.com>
Date: Wed, 2 Nov 2022 12:39:45 +0100
Subject: [PATCH] Fix bug in frange::contains_p() for signed zeros.

The contains_p() code wasn't returning true for non-singleton ranges
containing signed zeros.  With this patch we now handle:

	-0.0 exists in [-3, +5.0]
	+0.0 exists in [-3, +5.0]

gcc/ChangeLog:

	* value-range.cc (frange::contains_p): Fix signed zero handling.
	(range_tests_signed_zeros): New test.
---
 gcc/value-range.cc | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/gcc/value-range.cc b/gcc/value-range.cc
index 3743ec714b3..a855aaf626c 100644
--- a/gcc/value-range.cc
+++ b/gcc/value-range.cc
@@ -661,7 +661,7 @@ frange::contains_p (tree cst) const
     {
       // Make sure the signs are equal for signed zeros.
       if (HONOR_SIGNED_ZEROS (m_type) && real_iszero (rv))
-	return m_min.sign == m_max.sign && m_min.sign == rv->sign;
+	return rv->sign == m_min.sign || rv->sign == m_max.sign;
       return true;
     }
   return false;
@@ -3859,6 +3859,14 @@ range_tests_signed_zeros ()
   ASSERT_TRUE (r0.contains_p (neg_zero));
   ASSERT_FALSE (r0.contains_p (zero));
 
+  r0 = frange (neg_zero, zero);
+  ASSERT_TRUE (r0.contains_p (neg_zero));
+  ASSERT_TRUE (r0.contains_p (zero));
+
+  r0 = frange_float ("-3", "5");
+  ASSERT_TRUE (r0.contains_p (neg_zero));
+  ASSERT_TRUE (r0.contains_p (zero));
+
   // The intersection of zeros that differ in sign is a NAN (or
   // undefined if not honoring NANs).
   r0 = frange (neg_zero, neg_zero);
-- 
2.38.1


      reply	other threads:[~2022-11-02 13:35 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-15  5:40 Aldy Hernandez
2022-09-15  7:06 ` Richard Biener
2022-09-15 20:44   ` Aldy Hernandez
2022-09-16  8:33     ` Richard Sandiford
2022-09-16 13:26       ` Aldy Hernandez
2022-09-18  7:10         ` Aldy Hernandez
2022-09-27 13:00         ` Mikael Morin
2022-11-02 13:35           ` Aldy Hernandez [this message]

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=08ea88c3-df7a-acdd-1a75-b43d81225712@redhat.com \
    --to=aldyh@redhat.com \
    --cc=amacleod@redhat.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=morin-mikael@orange.fr \
    /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).