diff mbox series

Add inchash support for vrange.

Message ID 20230418090637.253140-2-aldyh@redhat.com
State New
Headers show
Series Add inchash support for vrange. | expand

Commit Message

Aldy Hernandez April 18, 2023, 9:06 a.m. UTC 
This patch provides inchash support for vrange.  It is along the lines
of the streaming support I just posted and will be used for IPA
hashing of ranges.

Thoughts?

gcc/ChangeLog:

	* inchash.cc (hash::add_real_value): New.
	* inchash.h (class hash): Add add_real_value.
	* value-range.cc (add_vrange): New.
	* value-range.h (inchash::add_vrange): New.
---
 gcc/inchash.cc     | 20 +++++++++++++++++++
 gcc/inchash.h      |  2 ++
 gcc/value-range.cc | 48 ++++++++++++++++++++++++++++++++++++++++++++++
 gcc/value-range.h  |  6 ++++++
 4 files changed, 76 insertions(+)

Comments

Aldy Hernandez April 18, 2023, 9:17 a.m. UTC | #1 
> +namespace inchash
> +{
> +
> +void
> +add_vrange (const vrange &v, inchash::hash &hstate,
> +	     unsigned int)
> +{
> +  if (v.undefined_p ())
> +    {
> +      hstate.add_int (VR_UNDEFINED);
> +      return;
> +    }
> +  // Types are ignored throughout to inhibit two ranges being equal
> +  // but having different hash values.  This can happen when two
> +  // ranges are equal types_compatible_p is true.

Sorry, this comment should read:

+  // Types are ignored throughout to inhibit two ranges being equal
+  // but having different hash values.  This can happen when two
+  // ranges are equal and their types are different (but
+  // types_compatible_p is true).
Jakub Jelinek April 18, 2023, 9:33 a.m. UTC | #2 
On Tue, Apr 18, 2023 at 11:06:38AM +0200, Aldy Hernandez via Gcc-patches wrote:
> This patch provides inchash support for vrange.  It is along the lines
> of the streaming support I just posted and will be used for IPA
> hashing of ranges.
> 
> Thoughts?
> 
> gcc/ChangeLog:
> 
> 	* inchash.cc (hash::add_real_value): New.
> 	* inchash.h (class hash): Add add_real_value.
> 	* value-range.cc (add_vrange): New.
> 	* value-range.h (inchash::add_vrange): New.
> ---
>  gcc/inchash.cc     | 20 +++++++++++++++++++
>  gcc/inchash.h      |  2 ++
>  gcc/value-range.cc | 48 ++++++++++++++++++++++++++++++++++++++++++++++
>  gcc/value-range.h  |  6 ++++++
>  4 files changed, 76 insertions(+)
> 
> diff --git a/gcc/inchash.cc b/gcc/inchash.cc
> index a30662b97fe..914e3cc92cd 100644
> --- a/gcc/inchash.cc
> +++ b/gcc/inchash.cc
> @@ -24,3 +24,23 @@ along with GCC; see the file COPYING3.  If not see
>  #endif
>  #include "system.h"
>  #include "coretypes.h"
> +#include "real.h"
> +#include "inchash.h"
> +
> +namespace inchash
> +{
> +
> +/* This is here instead of inchash.h to keep us from having to put
> +   real.h in coretypes.h.  */
> +void
> +hash::add_real_value (const real_value &v)
> +{
> +  add_int (v.sign);
> +  add_int (v.uexp);
> +  for (unsigned i = 0; i < SIGSZ; ++i)
> +    add_hwi (v.sig[i]);
> +  /* Ignore the rest of the flags, as sign, exponent, and
> +     significant bits should be enough.  */

I don't think that's the case.
At least cl, decimal and signalling are essential flags as well.
Dunno about canonical.
How do you otherwise differentiate between Inf and +0.0 or (canonical)
qNaN or (canonical) sNaN?
They have the same sign, uexp and sig.

	Jakub
Jakub Jelinek April 18, 2023, 10:32 a.m. UTC | #3 
On Tue, Apr 18, 2023 at 11:33:05AM +0200, Jakub Jelinek wrote:
> On Tue, Apr 18, 2023 at 11:06:38AM +0200, Aldy Hernandez via Gcc-patches wrote:
> > This patch provides inchash support for vrange.  It is along the lines
> > of the streaming support I just posted and will be used for IPA
> > hashing of ranges.
> > 
> > Thoughts?
> > 
> > gcc/ChangeLog:
> > 
> > 	* inchash.cc (hash::add_real_value): New.
> > 	* inchash.h (class hash): Add add_real_value.
> > 	* value-range.cc (add_vrange): New.
> > 	* value-range.h (inchash::add_vrange): New.
> > ---
> >  gcc/inchash.cc     | 20 +++++++++++++++++++
> >  gcc/inchash.h      |  2 ++
> >  gcc/value-range.cc | 48 ++++++++++++++++++++++++++++++++++++++++++++++
> >  gcc/value-range.h  |  6 ++++++
> >  4 files changed, 76 insertions(+)
> > 
> > diff --git a/gcc/inchash.cc b/gcc/inchash.cc
> > index a30662b97fe..914e3cc92cd 100644
> > --- a/gcc/inchash.cc
> > +++ b/gcc/inchash.cc
> > @@ -24,3 +24,23 @@ along with GCC; see the file COPYING3.  If not see
> >  #endif
> >  #include "system.h"
> >  #include "coretypes.h"
> > +#include "real.h"
> > +#include "inchash.h"
> > +
> > +namespace inchash
> > +{
> > +
> > +/* This is here instead of inchash.h to keep us from having to put
> > +   real.h in coretypes.h.  */
> > +void
> > +hash::add_real_value (const real_value &v)
> > +{
> > +  add_int (v.sign);
> > +  add_int (v.uexp);
> > +  for (unsigned i = 0; i < SIGSZ; ++i)
> > +    add_hwi (v.sig[i]);
> > +  /* Ignore the rest of the flags, as sign, exponent, and
> > +     significant bits should be enough.  */
> 
> I don't think that's the case.
> At least cl, decimal and signalling are essential flags as well.
> Dunno about canonical.
> How do you otherwise differentiate between Inf and +0.0 or (canonical)
> qNaN or (canonical) sNaN?
> They have the same sign, uexp and sig.

I'd say it is best to follow real_identical that is used for the
comparisons, that one always compares cl and sign and then differentiates
based on cl:
1) for rvc_zero/rvc_inf, everything else is ignored
2) for rvc_normal, decimal, uexp and sig are compared
3) for rvc_nan, signalling and canonical are compared and if !canonical,
   sig is also compared
So, perhaps:
  add_int (v.cl);
  add_int (v.sign);
  switch (v.cl)
    {
    case rvc_zero:
    case rvc_inf:
      return;
    case rvc_normal:
      add_int (v.decimal);
      add_int (REAL_EXP (&v));
      break;
    case rvc_nan:
      add_int (v.signalling);
      add_int (v.canonical);
      if (v.canonical)
	return;
      break;
    default:
      gcc_unreachable ();
    }
 for (unsigned i = 0; i < SIGSZ; ++i)
    add_hwi (v.sig[i]);

	Jakub
Aldy Hernandez April 18, 2023, 10:50 a.m. UTC | #4 
On 4/18/23 12:32, Jakub Jelinek wrote:
> On Tue, Apr 18, 2023 at 11:33:05AM +0200, Jakub Jelinek wrote:
>> On Tue, Apr 18, 2023 at 11:06:38AM +0200, Aldy Hernandez via Gcc-patches wrote:
>>> This patch provides inchash support for vrange.  It is along the lines
>>> of the streaming support I just posted and will be used for IPA
>>> hashing of ranges.
>>>
>>> Thoughts?
>>>
>>> gcc/ChangeLog:
>>>
>>> 	* inchash.cc (hash::add_real_value): New.
>>> 	* inchash.h (class hash): Add add_real_value.
>>> 	* value-range.cc (add_vrange): New.
>>> 	* value-range.h (inchash::add_vrange): New.
>>> ---
>>>   gcc/inchash.cc     | 20 +++++++++++++++++++
>>>   gcc/inchash.h      |  2 ++
>>>   gcc/value-range.cc | 48 ++++++++++++++++++++++++++++++++++++++++++++++
>>>   gcc/value-range.h  |  6 ++++++
>>>   4 files changed, 76 insertions(+)
>>>
>>> diff --git a/gcc/inchash.cc b/gcc/inchash.cc
>>> index a30662b97fe..914e3cc92cd 100644
>>> --- a/gcc/inchash.cc
>>> +++ b/gcc/inchash.cc
>>> @@ -24,3 +24,23 @@ along with GCC; see the file COPYING3.  If not see
>>>   #endif
>>>   #include "system.h"
>>>   #include "coretypes.h"
>>> +#include "real.h"
>>> +#include "inchash.h"
>>> +
>>> +namespace inchash
>>> +{
>>> +
>>> +/* This is here instead of inchash.h to keep us from having to put
>>> +   real.h in coretypes.h.  */
>>> +void
>>> +hash::add_real_value (const real_value &v)
>>> +{
>>> +  add_int (v.sign);
>>> +  add_int (v.uexp);
>>> +  for (unsigned i = 0; i < SIGSZ; ++i)
>>> +    add_hwi (v.sig[i]);
>>> +  /* Ignore the rest of the flags, as sign, exponent, and
>>> +     significant bits should be enough.  */
>>
>> I don't think that's the case.
>> At least cl, decimal and signalling are essential flags as well.
>> Dunno about canonical.
>> How do you otherwise differentiate between Inf and +0.0 or (canonical)
>> qNaN or (canonical) sNaN?
>> They have the same sign, uexp and sig.
> 
> I'd say it is best to follow real_identical that is used for the
> comparisons, that one always compares cl and sign and then differentiates
> based on cl:
> 1) for rvc_zero/rvc_inf, everything else is ignored
> 2) for rvc_normal, decimal, uexp and sig are compared
> 3) for rvc_nan, signalling and canonical are compared and if !canonical,
>     sig is also compared
> So, perhaps:
>    add_int (v.cl);
>    add_int (v.sign);
>    switch (v.cl)
>      {
>      case rvc_zero:
>      case rvc_inf:
>        return;
>      case rvc_normal:
>        add_int (v.decimal);
>        add_int (REAL_EXP (&v));
>        break;
>      case rvc_nan:
>        add_int (v.signalling);
>        add_int (v.canonical);
>        if (v.canonical)
> 	return;
>        break;
>      default:
>        gcc_unreachable ();
>      }
>   for (unsigned i = 0; i < SIGSZ; ++i)
>      add_hwi (v.sig[i]);
> 
> 	Jakub
> 

Sounds good.  Thanks for the patch.

OK pending tests?

p.s. Cleaned up the need for declaring add_vrange() a friend of frange 
now that we have a way of getting the nan_state elegantly.
Jakub Jelinek April 18, 2023, 10:59 a.m. UTC | #5 
On Tue, Apr 18, 2023 at 12:50:58PM +0200, Aldy Hernandez wrote:
> --- a/gcc/value-range.cc
> +++ b/gcc/value-range.cc
> @@ -232,6 +232,58 @@ vrange::dump (FILE *file) const
>    pp_flush (&buffer);
>  }
>  
> +namespace inchash
> +{
> +
> +void
> +add_vrange (const vrange &v, inchash::hash &hstate,
> +	     unsigned int)
> +{
> +  if (v.undefined_p ())
> +    {
> +      hstate.add_int (VR_UNDEFINED);
> +      return;
> +    }
> +  // Types are ignored throughout to inhibit two ranges being equal
> +  // but having different hash values.  This can happen when two
> +  // ranges are equal and their types are different (but
> +  // types_compatible_p is true).
> +  if (is_a <irange> (v))
> +    {
> +      const irange &r = as_a <irange> (v);
> +      if (r.varying_p ())
> +	hstate.add_int (VR_VARYING);
> +      else
> +	hstate.add_int (VR_RANGE);

Shouldn't this also
      hstate.add_int (r.num_pairs ());
?
Or is that unnecessary because different number of add_wide_int
calls will likely result in different hashes then?

Otherwise LGTM.

> +      for (unsigned i = 0; i < r.num_pairs (); ++i)
> +	{
> +	  hstate.add_wide_int (r.lower_bound (i));
> +	  hstate.add_wide_int (r.upper_bound (i));
> +	}
> +      hstate.add_wide_int (r.get_nonzero_bits ());
> +      return;
> +    }

	Jakub
Aldy Hernandez April 18, 2023, 11:33 a.m. UTC | #6 
On 4/18/23 12:59, Jakub Jelinek wrote:
> On Tue, Apr 18, 2023 at 12:50:58PM +0200, Aldy Hernandez wrote:
>> --- a/gcc/value-range.cc
>> +++ b/gcc/value-range.cc
>> @@ -232,6 +232,58 @@ vrange::dump (FILE *file) const
>>     pp_flush (&buffer);
>>   }
>>   
>> +namespace inchash
>> +{
>> +
>> +void
>> +add_vrange (const vrange &v, inchash::hash &hstate,
>> +	     unsigned int)
>> +{
>> +  if (v.undefined_p ())
>> +    {
>> +      hstate.add_int (VR_UNDEFINED);
>> +      return;
>> +    }
>> +  // Types are ignored throughout to inhibit two ranges being equal
>> +  // but having different hash values.  This can happen when two
>> +  // ranges are equal and their types are different (but
>> +  // types_compatible_p is true).
>> +  if (is_a <irange> (v))
>> +    {
>> +      const irange &r = as_a <irange> (v);
>> +      if (r.varying_p ())
>> +	hstate.add_int (VR_VARYING);
>> +      else
>> +	hstate.add_int (VR_RANGE);
> 
> Shouldn't this also
>        hstate.add_int (r.num_pairs ());
> ?
> Or is that unnecessary because different number of add_wide_int
> calls will likely result in different hashes then?

That was my thinking, and we could save one write.

I can add the num_pairs() if you prefer.  I don't have a strong opinion.

Aldy
Jakub Jelinek April 18, 2023, 11:34 a.m. UTC | #7 
On Tue, Apr 18, 2023 at 01:33:47PM +0200, Aldy Hernandez wrote:
> > > +      const irange &r = as_a <irange> (v);
> > > +      if (r.varying_p ())
> > > +	hstate.add_int (VR_VARYING);
> > > +      else
> > > +	hstate.add_int (VR_RANGE);
> > 
> > Shouldn't this also
> >        hstate.add_int (r.num_pairs ());
> > ?
> > Or is that unnecessary because different number of add_wide_int
> > calls will likely result in different hashes then?
> 
> That was my thinking, and we could save one write.
> 
> I can add the num_pairs() if you prefer.  I don't have a strong opinion.

Me neither.  Let's go with your version then.

	Jakub
diff mbox series

Patch

diff --git a/gcc/inchash.cc b/gcc/inchash.cc
index a30662b97fe..914e3cc92cd 100644
--- a/gcc/inchash.cc
+++ b/gcc/inchash.cc
@@ -24,3 +24,23 @@  along with GCC; see the file COPYING3.  If not see
 #endif
 #include "system.h"
 #include "coretypes.h"
+#include "real.h"
+#include "inchash.h"
+
+namespace inchash
+{
+
+/* This is here instead of inchash.h to keep us from having to put
+   real.h in coretypes.h.  */
+void
+hash::add_real_value (const real_value &v)
+{
+  add_int (v.sign);
+  add_int (v.uexp);
+  for (unsigned i = 0; i < SIGSZ; ++i)
+    add_hwi (v.sig[i]);
+  /* Ignore the rest of the flags, as sign, exponent, and
+     significant bits should be enough.  */
+}
+
+} // namespace inchash
diff --git a/gcc/inchash.h b/gcc/inchash.h
index bf76308431d..41ae153d1c5 100644
--- a/gcc/inchash.h
+++ b/gcc/inchash.h
@@ -88,6 +88,8 @@  class hash
       add_hwi (x.sext_elt (i));
   }
 
+  void add_real_value (const class real_value &v);
+
   /* Hash in pointer PTR.  */
   void add_ptr (const void *ptr)
   {
diff --git a/gcc/value-range.cc b/gcc/value-range.cc
index 963330eed79..31ec832e185 100644
--- a/gcc/value-range.cc
+++ b/gcc/value-range.cc
@@ -232,6 +232,54 @@  vrange::dump (FILE *file) const
   pp_flush (&buffer);
 }
 
+namespace inchash
+{
+
+void
+add_vrange (const vrange &v, inchash::hash &hstate,
+	     unsigned int)
+{
+  if (v.undefined_p ())
+    {
+      hstate.add_int (VR_UNDEFINED);
+      return;
+    }
+  // Types are ignored throughout to inhibit two ranges being equal
+  // but having different hash values.  This can happen when two
+  // ranges are equal types_compatible_p is true.
+  if (is_a <irange> (v))
+    {
+      const irange &r = as_a <irange> (v);
+      if (r.varying_p ())
+	hstate.add_int (VR_VARYING);
+      else
+	hstate.add_int (VR_RANGE);
+      for (unsigned i = 0; i < r.num_pairs (); ++i)
+	{
+	  hstate.add_wide_int (r.lower_bound (i));
+	  hstate.add_wide_int (r.upper_bound (i));
+	}
+      hstate.add_wide_int (r.get_nonzero_bits ());
+      return;
+    }
+  if (is_a <frange> (v))
+    {
+      const frange &r = as_a <frange> (v);
+      if (r.varying_p ())
+	hstate.add_int (VR_VARYING);
+      else
+	hstate.add_int (VR_RANGE);
+      hstate.add_real_value (r.m_min);
+      hstate.add_real_value (r.m_max);
+      hstate.add_int (r.m_pos_nan);
+      hstate.add_int (r.m_neg_nan);
+      return;
+    }
+  gcc_unreachable ();
+}
+
+} //namespace inchash
+
 bool
 irange::supports_type_p (const_tree type) const
 {
diff --git a/gcc/value-range.h b/gcc/value-range.h
index e56df9db901..cb7418208d4 100644
--- a/gcc/value-range.h
+++ b/gcc/value-range.h
@@ -109,6 +109,11 @@  protected:
   const ENUM_BITFIELD(value_range_discriminator) m_discriminator : 4;
 };
 
+namespace inchash
+{
+  extern void add_vrange (const vrange &, hash &, unsigned flags = 0);
+}
+
 // An integer range without any storage.
 
 class GTY((user)) irange : public vrange
@@ -332,6 +337,7 @@  class frange : public vrange
   friend void streamer_read_value_range (class lto_input_block *,
 					 class data_in *,
 					 class Value_Range &);
+  friend void inchash::add_vrange (const vrange &, inchash::hash &, unsigned);
 public:
   frange ();
   frange (const frange &);