Re: [PATCH] Cleanup irange::set.

[Richard Sandiford via Gcc-patches]

Aldy Hernandez <al...@redhat.com> writes:
>> (actually I can see 3245 ICEs on aarch64)
>> 
>> Can you fix it?
>
> Sure can.
>
> Richard, I seem to have incorrectly removed the early exit for varying, 
> and that affected the changes you made for poly ints.  Is there any 
> reason we can't just exit and set varying without checking for kind != 
> VR_VARYING?

No reason, it seemed more natural to drop to a lower kind with the old
code :-)  (But not with the new code.)

But it isn't obvious to me why the code is now structed the way it is.

  if (POLY_INT_CST_P (min) || POLY_INT_CST_P (max))
    {
      set_varying (TREE_TYPE (min));
      return;
    }

  // Nothing to canonicalize for symbolic ranges.
  if (TREE_CODE (min) != INTEGER_CST
      || TREE_CODE (max) != INTEGER_CST)
    {
      m_kind = kind;
      m_base[0] = min;
      m_base[1] = max;
      m_num_ranges = 1;
      return;
    }

  swap_out_of_order_endpoints (min, max, kind);
  if (kind == VR_VARYING)
    {
      set_varying (TREE_TYPE (min));
      return;
    }

Why do we want to check “min” and “max” being INTEGER_CST before “kind”
being VR_VARYING, and the potentially record VR_VARYING with specific
bounds?  And why do we want to swap the “min” and “max” before checking
whether “kind” is VR_VARYING (when we'll then drop the min and max anyway)?
I think this would benefit from a bit more commentary at least.

Thanks,
Richard


>
> The attached patch fixes the problem for aarch64.
>
> Testing in progress...
> Aldy
>
>      Early exit from irange::set for poly ints.
>
>      My previous cleanups to irange::set moved the early exit when
>      VARYING.  This caused poly int varyings to be created with
>      incorrect min/max.
>
>      We can just set varying and exit for all poly ints.
>
>      gcc/ChangeLog:
>
>              * value-range.cc (irange::set): Early exit for poly ints.
>
> diff --git a/gcc/value-range.cc b/gcc/value-range.cc
> index 61f7da278d6..178ef1b0a4f 100644
> --- a/gcc/value-range.cc
> +++ b/gcc/value-range.cc
> @@ -249,9 +249,11 @@ irange::set (tree min, tree max, value_range_kind kind)
>         return;
>       }
>
> -  if (kind != VR_VARYING
> -      && (POLY_INT_CST_P (min) || POLY_INT_CST_P (max)))
> -    kind = VR_VARYING;
> +  if (POLY_INT_CST_P (min) || POLY_INT_CST_P (max))
> +    {
> +      set_varying (TREE_TYPE (min));
> +      return;
> +    }
>
>     // Nothing to canonicalize for symbolic ranges.
>     if (TREE_CODE (min) != INTEGER_CST