* [PATCH] Allow copying of symbolic ranges to an irange.
@ 2020-09-15 15:57 Aldy Hernandez
2020-09-15 19:59 ` Andrew MacLeod
0 siblings, 1 reply; 4+ messages in thread
From: Aldy Hernandez @ 2020-09-15 15:57 UTC (permalink / raw)
To: Andrew MacLeod, gcc-patches
This fixes an ICE when trying to copy a legacy value_range containing a
symbolic to a multi-range:
min = make_ssa_name (type);
max = build_int_cst (type, 55);
value_range vv (min, max);
int_range<2> vr = vv;
This doesn't affect anything currently, as we don't have a lot of
interactions between value_range's and multi_range's in trunk right, but
it will become a problem as soon as someone tries to get a range from
evrp and copy it over to a multi-range.
OK pending tests?
gcc/ChangeLog:
* range-op.cc (multi_precision_range_tests): Normalize symbolics when
copying to a multi-range.
* value-range.cc (irange::copy_legacy_range): Add test.
---
gcc/range-op.cc | 9 +++++++++
gcc/value-range.cc | 12 +++++++++++-
2 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/gcc/range-op.cc b/gcc/range-op.cc
index c5f511422f4..8e52d5318e9 100644
--- a/gcc/range-op.cc
+++ b/gcc/range-op.cc
@@ -3463,6 +3463,15 @@ multi_precision_range_tests ()
small = big;
ASSERT_TRUE (small == int_range<1> (INT (21), INT (21), VR_ANTI_RANGE));
+ // Copying a legacy symbolic to an int_range should normalize the
+ // symbolic at copy time.
+ {
+ value_range legacy_range (make_ssa_name (integer_type_node), INT (25));
+ int_range<2> copy = legacy_range;
+ ASSERT_TRUE (copy == int_range<2> (vrp_val_min (integer_type_node),
+ INT (25)));
+ }
+
range3_tests ();
}
diff --git a/gcc/value-range.cc b/gcc/value-range.cc
index 20aa4f114c9..26ccd143e5c 100644
--- a/gcc/value-range.cc
+++ b/gcc/value-range.cc
@@ -101,7 +101,17 @@ irange::copy_legacy_range (const irange &src)
VR_ANTI_RANGE);
}
else
- set (src.min (), src.max (), VR_RANGE);
+ {
+ // If copying legacy to int_range, normalize any symbolics.
+ if (src.legacy_mode_p () && !range_has_numeric_bounds_p (&src))
+ {
+ value_range cst (src);
+ cst.normalize_symbolics ();
+ set (cst.min (), cst.max ());
+ return;
+ }
+ set (src.min (), src.max ());
+ }
}
// Swap min/max if they are out of order. Return TRUE if further
--
2.26.2
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Allow copying of symbolic ranges to an irange.
2020-09-15 15:57 [PATCH] Allow copying of symbolic ranges to an irange Aldy Hernandez
@ 2020-09-15 19:59 ` Andrew MacLeod
2020-09-16 16:25 ` Aldy Hernandez
0 siblings, 1 reply; 4+ messages in thread
From: Andrew MacLeod @ 2020-09-15 19:59 UTC (permalink / raw)
To: Aldy Hernandez, gcc-patches
On 9/15/20 11:57 AM, Aldy Hernandez wrote:
> This fixes an ICE when trying to copy a legacy value_range containing
> a symbolic to a multi-range:
>
> min = make_ssa_name (type);
> max = build_int_cst (type, 55);
> value_range vv (min, max);
> int_range<2> vr = vv;
>
> This doesn't affect anything currently, as we don't have a lot of
> interactions between value_range's and multi_range's in trunk right,
> but it will become a problem as soon as someone tries to get a range
> from evrp and copy it over to a multi-range.
>
> OK pending tests?
>
> gcc/ChangeLog:
>
> * range-op.cc (multi_precision_range_tests): Normalize symbolics
> when copying to a multi-range.
> * value-range.cc (irange::copy_legacy_range): Add test.
> ---
> gcc/range-op.cc | 9 +++++++++
> gcc/value-range.cc | 12 +++++++++++-
> 2 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/range-op.cc b/gcc/range-op.cc
> index c5f511422f4..8e52d5318e9 100644
> --- a/gcc/range-op.cc
> +++ b/gcc/range-op.cc
> @@ -3463,6 +3463,15 @@ multi_precision_range_tests ()
> small = big;
> ASSERT_TRUE (small == int_range<1> (INT (21), INT (21),
> VR_ANTI_RANGE));
>
> + // Copying a legacy symbolic to an int_range should normalize the
> + // symbolic at copy time.
> + {
> + value_range legacy_range (make_ssa_name (integer_type_node), INT
> (25));
> + int_range<2> copy = legacy_range;
> + ASSERT_TRUE (copy == int_range<2> (vrp_val_min (integer_type_node),
> + INT (25)));
> + }
> +
> range3_tests ();
> }
>
> diff --git a/gcc/value-range.cc b/gcc/value-range.cc
> index 20aa4f114c9..26ccd143e5c 100644
> --- a/gcc/value-range.cc
> +++ b/gcc/value-range.cc
> @@ -101,7 +101,17 @@ irange::copy_legacy_range (const irange &src)
> VR_ANTI_RANGE);
> }
> else
> - set (src.min (), src.max (), VR_RANGE);
> + {
> + // If copying legacy to int_range, normalize any symbolics.
> + if (src.legacy_mode_p () && !range_has_numeric_bounds_p (&src))
> + {
> + value_range cst (src);
> + cst.normalize_symbolics ();
> + set (cst.min (), cst.max ());
> + return;
> + }
> + set (src.min (), src.max ());
> + }
> }
>
> // Swap min/max if they are out of order. Return TRUE if further
these seems OK, but can't there be anti-ranges with symbolics too? ie
~[a_12, a_12]
The code for that just does:
else if (src.kind () == VR_ANTI_RANGE)
set (src.min (), src.max (), VR_ANTI_RANGE);
That should just go to varying I guess?
The conversion to legacy anti-range code also seems a little suspect in
some cases...
Finally, we theoretically shouldn't be accessing 'min()' and 'max()'
fields in a multirange, which also looks like might happen in the final
else clause.
I wonder if it might be less complex to simply have 2 routines, like
copy_to_legacy() and copy_from_legacy() instead of trying to handle
then together? I do find it seems to require more thinking than it
should to follow the cases :-)
Andrew
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Allow copying of symbolic ranges to an irange.
2020-09-15 19:59 ` Andrew MacLeod
@ 2020-09-16 16:25 ` Aldy Hernandez
2020-09-16 18:43 ` Andrew MacLeod
0 siblings, 1 reply; 4+ messages in thread
From: Aldy Hernandez @ 2020-09-16 16:25 UTC (permalink / raw)
To: Andrew MacLeod, gcc-patches
>> // Swap min/max if they are out of order. Return TRUE if further
> these seems OK, but can't there be anti-ranges with symbolics too? ie
> ~[a_12, a_12]
> The code for that just does:
>
> else if (src.kind () == VR_ANTI_RANGE)
> set (src.min (), src.max (), VR_ANTI_RANGE);
>
> That should just go to varying I guess?
Ah, you're right. I've tweaked the patch and have added a corresponding
test.
>
> The conversion to legacy anti-range code also seems a little suspect in
> some cases...
>
> Finally, we theoretically shouldn't be accessing 'min()' and 'max()'
> fields in a multirange, which also looks like might happen in the final
> else clause.
>
> I wonder if it might be less complex to simply have 2 routines, like
> copy_to_legacy() and copy_from_legacy() instead of trying to handle
> then together? I do find it seems to require more thinking than it
> should to follow the cases :-)
Sigh, yes. That code is too clever for its own good. I'll work on it
as a follow-up.
OK pending tests?
Aldy
gcc/ChangeLog:
* range-op.cc (multi_precision_range_tests): Normalize symbolics when
copying to a multi-range.
* value-range.cc (irange::copy_legacy_range): Add test.
---
gcc/range-op.cc | 15 +++++++++++++++
gcc/value-range.cc | 19 +++++++++++++++++--
2 files changed, 32 insertions(+), 2 deletions(-)
diff --git a/gcc/range-op.cc b/gcc/range-op.cc
index fbf78be0a3c..3ab268f101e 100644
--- a/gcc/range-op.cc
+++ b/gcc/range-op.cc
@@ -3453,6 +3453,21 @@ multi_precision_range_tests ()
small = big;
ASSERT_TRUE (small == int_range<1> (INT (21), INT (21), VR_ANTI_RANGE));
+ // Copying a legacy symbolic to an int_range should normalize the
+ // symbolic at copy time.
+ {
+ tree ssa = make_ssa_name (integer_type_node);
+ value_range legacy_range (ssa, INT (25));
+ int_range<2> copy = legacy_range;
+ ASSERT_TRUE (copy == int_range<2> (vrp_val_min (integer_type_node),
+ INT (25)));
+
+ // Test that copying ~[abc_23, abc_23] to a multi-range yields varying.
+ legacy_range = value_range (ssa, ssa, VR_ANTI_RANGE);
+ copy = legacy_range;
+ ASSERT_TRUE (copy.varying_p ());
+ }
+
range3_tests ();
}
diff --git a/gcc/value-range.cc b/gcc/value-range.cc
index 20aa4f114c9..ed2c322ded9 100644
--- a/gcc/value-range.cc
+++ b/gcc/value-range.cc
@@ -92,7 +92,12 @@ irange::copy_legacy_range (const irange &src)
else if (src.varying_p ())
set_varying (src.type ());
else if (src.kind () == VR_ANTI_RANGE)
- set (src.min (), src.max (), VR_ANTI_RANGE);
+ {
+ if (src.legacy_mode_p () && !range_has_numeric_bounds_p (&src))
+ set_varying (src.type ());
+ else
+ set (src.min (), src.max (), VR_ANTI_RANGE);
+ }
else if (legacy_mode_p () && src.maybe_anti_range ())
{
int_range<3> tmp (src);
@@ -101,7 +106,17 @@ irange::copy_legacy_range (const irange &src)
VR_ANTI_RANGE);
}
else
- set (src.min (), src.max (), VR_RANGE);
+ {
+ // If copying legacy to int_range, normalize any symbolics.
+ if (src.legacy_mode_p () && !range_has_numeric_bounds_p (&src))
+ {
+ value_range cst (src);
+ cst.normalize_symbolics ();
+ set (cst.min (), cst.max ());
+ return;
+ }
+ set (src.min (), src.max ());
+ }
}
// Swap min/max if they are out of order. Return TRUE if further
--
2.26.2
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Allow copying of symbolic ranges to an irange.
2020-09-16 16:25 ` Aldy Hernandez
@ 2020-09-16 18:43 ` Andrew MacLeod
0 siblings, 0 replies; 4+ messages in thread
From: Andrew MacLeod @ 2020-09-16 18:43 UTC (permalink / raw)
To: Aldy Hernandez, gcc-patches
On 9/16/20 12:25 PM, Aldy Hernandez wrote:
>
>
> >> // Swap min/max if they are out of order. Return TRUE if further
> > these seems OK, but can't there be anti-ranges with symbolics too? ie
> > ~[a_12, a_12]
> > The code for that just does:
> >
> > else if (src.kind () == VR_ANTI_RANGE)
> > set (src.min (), src.max (), VR_ANTI_RANGE);
> >
> > That should just go to varying I guess?
>
> Ah, you're right. I've tweaked the patch and have added a
> corresponding test.
>
> >
> > The conversion to legacy anti-range code also seems a little suspect in
> > some cases...
> >
> > Finally, we theoretically shouldn't be accessing 'min()' and 'max()'
> > fields in a multirange, which also looks like might happen in the final
> > else clause.
> >
> > I wonder if it might be less complex to simply have 2 routines, like
> > copy_to_legacy() and copy_from_legacy() instead of trying to handle
> > then together? I do find it seems to require more thinking than it
> > should to follow the cases :-)
>
> Sigh, yes. That code is too clever for its own good. I'll work on it
> as a follow-up.
>
> OK pending tests?
>
OK. but do follow it up.
Andrew
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-09-16 18:43 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-15 15:57 [PATCH] Allow copying of symbolic ranges to an irange Aldy Hernandez
2020-09-15 19:59 ` Andrew MacLeod
2020-09-16 16:25 ` Aldy Hernandez
2020-09-16 18:43 ` Andrew MacLeod
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).