Add inchash support for vrange.
Commit Message
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
> +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).
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
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
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.
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
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
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
@@ -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
@@ -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)
{
@@ -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
{
@@ -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 &);