[COMMITTED] Denormalize VR_VARYING to VR_RANGE before passing it to set_range_info_raw.

[COMMITTED] Denormalize VR_VARYING to VR_RANGE before passing it to set_range_info_raw.

[Aldy Hernandez]

We are ICEing in set_range_info_raw because value_range_kind cannot be
VR_VARYING, since SSA_NAME_RANGE_TYPE can only hold VR_RANGE /
VR_ANTI_RANGE.  Most of the time setting a VR_VARYING as a global
range makes no sense.  However, we can have a range spanning the
entire domain (VR_RANGE of [MIN,MAX] which is essentially a
VR_VARYING), if the nonzero bits are set.

This was working before because set_range_info_raw allows setting
VR_RANGE of [MIN, MAX].  However, when going through an irange, we
normalize this to a VR_VARYING, thus causing the ICE.  It's
interesting that other calls to set_range_info with an irange haven't
triggered this.

One solution would be to just ignore VR_VARYING and bail, since
set_range_info* is really an update of the current range semantic
wise.  After all, we keep the nonzero bits which provide additional
info.  But this would be a change in behavior, so not suitable until
after GCC 12 is released.  So in order to keep with current behavior
we can just denormalize the varying to VR_RANGE.

Tested on x86-64 Linux.

	    PR tree-optimization/105432

gcc/ChangeLog:

	* tree-ssanames.cc (set_range_info): Denormalize VR_VARYING to
	VR_RANGE before passing a piecewise range to set_range_info_raw.
---
 gcc/tree-ssanames.cc | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/gcc/tree-ssanames.cc b/gcc/tree-ssanames.cc
index c957597af4f..05536cd2f74 100644
--- a/gcc/tree-ssanames.cc
+++ b/gcc/tree-ssanames.cc
@@ -395,8 +395,17 @@ set_range_info (tree name, enum value_range_kind range_type,
 {
   gcc_assert (!POINTER_TYPE_P (TREE_TYPE (name)));
 
-  /* A range of the entire domain is really no range at all.  */
   tree type = TREE_TYPE (name);
+  if (range_type == VR_VARYING)
+    {
+      /* SSA_NAME_RANGE_TYPE can only hold a VR_RANGE or
+	 VR_ANTI_RANGE.  Denormalize VR_VARYING to VR_RANGE.  */
+      range_type = VR_RANGE;
+      gcc_checking_assert (min == wi::min_value (type));
+      gcc_checking_assert (max == wi::max_value (type));
+    }
+
+  /* A range of the entire domain is really no range at all.  */
   if (min == wi::min_value (TYPE_PRECISION (type), TYPE_SIGN (type))
       && max == wi::max_value (TYPE_PRECISION (type), TYPE_SIGN (type)))
     {
-- 
2.35.1

Re: [COMMITTED] Denormalize VR_VARYING to VR_RANGE before passing it to set_range_info_raw.

[Richard Biener]

On Sun, May 1, 2022 at 2:15 PM Aldy Hernandez via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> We are ICEing in set_range_info_raw because value_range_kind cannot be
> VR_VARYING, since SSA_NAME_RANGE_TYPE can only hold VR_RANGE /
> VR_ANTI_RANGE.  Most of the time setting a VR_VARYING as a global
> range makes no sense.  However, we can have a range spanning the
> entire domain (VR_RANGE of [MIN,MAX] which is essentially a
> VR_VARYING), if the nonzero bits are set.
>
> This was working before because set_range_info_raw allows setting
> VR_RANGE of [MIN, MAX].  However, when going through an irange, we
> normalize this to a VR_VARYING, thus causing the ICE.  It's
> interesting that other calls to set_range_info with an irange haven't
> triggered this.
>
> One solution would be to just ignore VR_VARYING and bail, since
> set_range_info* is really an update of the current range semantic
> wise.  After all, we keep the nonzero bits which provide additional
> info.  But this would be a change in behavior, so not suitable until
> after GCC 12 is released.  So in order to keep with current behavior
> we can just denormalize the varying to VR_RANGE.

But there's no point in storing such info - shouldn't the function simply
clear the range and return? Or at least avoid allocating a new range
-info and just return if none is associated with the SSA name at the moment?

> Tested on x86-64 Linux.
>
>             PR tree-optimization/105432
>
> gcc/ChangeLog:
>
>         * tree-ssanames.cc (set_range_info): Denormalize VR_VARYING to
>         VR_RANGE before passing a piecewise range to set_range_info_raw.
> ---
>  gcc/tree-ssanames.cc | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/tree-ssanames.cc b/gcc/tree-ssanames.cc
> index c957597af4f..05536cd2f74 100644
> --- a/gcc/tree-ssanames.cc
> +++ b/gcc/tree-ssanames.cc
> @@ -395,8 +395,17 @@ set_range_info (tree name, enum value_range_kind range_type,
>  {
>    gcc_assert (!POINTER_TYPE_P (TREE_TYPE (name)));
>
> -  /* A range of the entire domain is really no range at all.  */
>    tree type = TREE_TYPE (name);
> +  if (range_type == VR_VARYING)
> +    {
> +      /* SSA_NAME_RANGE_TYPE can only hold a VR_RANGE or
> +        VR_ANTI_RANGE.  Denormalize VR_VARYING to VR_RANGE.  */
> +      range_type = VR_RANGE;
> +      gcc_checking_assert (min == wi::min_value (type));
> +      gcc_checking_assert (max == wi::max_value (type));
> +    }
> +
> +  /* A range of the entire domain is really no range at all.  */
>    if (min == wi::min_value (TYPE_PRECISION (type), TYPE_SIGN (type))
>        && max == wi::max_value (TYPE_PRECISION (type), TYPE_SIGN (type)))
>      {
> --
> 2.35.1
>

Re: [COMMITTED] Denormalize VR_VARYING to VR_RANGE before passing it to set_range_info_raw.

[Aldy Hernandez]

On Mon, May 2, 2022 at 8:34 AM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Sun, May 1, 2022 at 2:15 PM Aldy Hernandez via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > We are ICEing in set_range_info_raw because value_range_kind cannot be
> > VR_VARYING, since SSA_NAME_RANGE_TYPE can only hold VR_RANGE /
> > VR_ANTI_RANGE.  Most of the time setting a VR_VARYING as a global
> > range makes no sense.  However, we can have a range spanning the
> > entire domain (VR_RANGE of [MIN,MAX] which is essentially a
> > VR_VARYING), if the nonzero bits are set.
> >
> > This was working before because set_range_info_raw allows setting
> > VR_RANGE of [MIN, MAX].  However, when going through an irange, we
> > normalize this to a VR_VARYING, thus causing the ICE.  It's
> > interesting that other calls to set_range_info with an irange haven't
> > triggered this.
> >
> > One solution would be to just ignore VR_VARYING and bail, since
> > set_range_info* is really an update of the current range semantic
> > wise.  After all, we keep the nonzero bits which provide additional
> > info.  But this would be a change in behavior, so not suitable until
> > after GCC 12 is released.  So in order to keep with current behavior
> > we can just denormalize the varying to VR_RANGE.
>
> But there's no point in storing such info - shouldn't the function simply
> clear the range and return? Or at least avoid allocating a new range
> -info and just return if none is associated with the SSA name at the moment?

Yup.  That's what the code block immediately following will do: clear
the range (and free the storage), but only if the nonzero bits are
also clear.

This duality of ranges with nonzero bits and ranges is a bit annoying.
I have code overhauling this for when we provide a way to store global
(multi) ranges later this cycle.  Nonzero bits will live in the range
itself, and updating the global range / nonzero bits will have one
entry point that takes a range.  Also, nonzero bits and ranges will be
kept up to date wrt each other automatically.

Aldy