Allow copying of symbolic ranges to an irange.

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(-)

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

>>  // 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

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