* [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).