Re: [PATCH] Fixup possible VR_ANTI_RANGE value_range issues

[Richard Biener <rguenther@suse.de>]

On Mon, 27 Feb 2023, Aldy Hernandez wrote:

> 
> 
> On 2/27/23 14:58, Richard Biener wrote:
> > After fixing PR107561 the following avoids looking at VR_ANTI_RANGE
> > ranges where it doesn't seem obvious the code does the correct
> > thing here (lower_bound and upper_bound do not work as expected).
> 
> I do realize there's some confusion here, and some of it is my fault. This has
> become obvious in my upcoming work removing all of legacy.
> 
> What's going on is that ultimately min/max are broken with (non-legacy)
> iranges.  Or at the very least inconsistent between legacy and non-legacy.
> These are left over from the legacy world, and have been marked DEPRECATED for
> a few releases, but the middle end warnings continued to use them and even
> added new uses after they were obsoleted.
> 
> min/max have different meanings depending on kind(), which is also deprecated,
> btw.  They are the underlying min/max fields from the legacy implementation,
> and thus somewhat leak the implementation details. Unfortunately, they are
> being called from non-legacy code which is ignoring the kind() field.
> 
> In retrospect I should've converted everything away from min/max/kind years
> ago, or at the very least converted min/max to work with non-legacy more
> consistently.
> 
> For the record:
> 
>   enum value_range_kind kind () const;		// DEPRECATED
>   tree min () const;				// DEPRECATED
>   tree max () const;				// DEPRECATED
>   bool symbolic_p () const;			// DEPRECATED
>   bool constant_p () const;			// DEPRECATED
>   void normalize_symbolics ();			// DEPRECATED
>   void normalize_addresses ();			// DEPRECATED
>   bool may_contain_p (tree) const;		// DEPRECATED
>   bool legacy_verbose_union_ (const class irange *);	// DEPRECATED
>   bool legacy_verbose_intersect (const irange *);	// DEPRECATED
> 
> In my local branch I tried converting all the middle-end legacy API uses to
> the new API, but a bunch of tests started failing, and lots more false
> positives started showing up in correct code.  I suspect that's part of the
> reason legacy continued to be used in these passes :-/.  As you point out in
> the PR, the tests seem designed to test the current (at times broken)
> implementation.
> 
> That being said, the 5 fixes in your patch are not wrong to begin with,
> because all uses guard lower_bound() and upper_bound() which work correctly.
> They return the lowest and uppermost possible bound for the range (ignoring
> the underlying implementation).  So, the lower bound of a signed non-zero is
> -INF because ~[0,0] is really [-INF,-1][1,+INF]. In the min/max legacy code,
> min() of signed non-zero (~[0,0]) is 0.  The new implementation has no concept
> of anti-ranges, and we don't leak any of that out.
> 
> Any uses of min/max without looking at kind() are definitely broken. OTOH uses
> of lower/upper_bound are fine and should work with both legacy and non-legacy.
> 
> Unrelated, but one place where I haven't been able to convince myself that the
> use is correct is bounds_of_var_in_loop:
> 
> > /* Check if init + nit * step overflows.  Though we checked
> >        scev {init, step}_loop doesn't wrap, it is not enough
> >        because the loop may exit immediately.  Overflow could
> >        happen in the plus expression in this case.  */
> >     if ((dir == EV_DIR_DECREASES
> >          && compare_values (maxvr.min (), initvr.min ()) != -1)
> >         || (dir == EV_DIR_GROWS
> >      && compare_values (maxvr.max (), initvr.max ()) != 1))
> 
> Ughh...this is all slated to go away, and I have patches removing all of
> legacy and the old API.
> 
> Does this help?  Do you still think lower and upper bound are not working as
> expected?

lower_bound/upper_bound work as expected, 
tree_lower_bound/tree_upper_bound do not.  I've checked and all uses
I "fixed" use lower_bound/upper_boud.

tree_lower_bound & friends are 'protected', but in the above light the
comment

  // potential promotion to public?

looks dangerous (I was considering using them ...).

I'm dropping the patch, it's probably time to work on getting rid of
'value_range' uses (aka legacy_mode_p ()) and/or replace raw accesses
to the min/max for that mode with m_base accesses?  At least
that there's tree_{upper,lower}_bound for pair != 0 suggests this
function is used with differing semantics internally which is a
source of confusion (to me at least).  tree_{lower,upper}_bound
should be able to assert it's not a legacy mode range and
min/max should be able to assert that it is.

Btw, legacy_upper_bound looks wrong:

  if (m_kind == VR_ANTI_RANGE)
    {
      tree typ = type (), t;
      if (pair == 1 || vrp_val_is_min (min ()))
        t = vrp_val_max (typ);
      else
        t = wide_int_to_tree (typ, wi::to_wide (min ()) - 1);

not sure what the pair == 1 case is about, but the upper bound
of an anti-range is only special when max() is vrp_val_is_max,
aka ~[low, +INF] where it then is low - 1, but the above checks
for ~[-INF, high] and returns +INF in that case.  That's correct
unless high == +INF at least, but the else case is wrong for
all high != +INF?

I think it should be

      if (pair == 1 || !vrp_val_is_max (max ()))
        t = vrp_val_max (typ);
      else
        t = wide_int_to_tree (typ, wi::to_wide (min ()) - 1);

no?

The pair == 1 case is a mystery to me - I suppose we consider the
non-existing range to be empty?

Richard.


> Aldy
> 
> > 
> > Bootstrapped and tested on x86_64-unknown-linux-gnu.
> > 
> > OK?
> > 
> > Thanks,
> > Richard.
> > 
> >  * gimple-ssa-sprintf.cc (get_int_range): Avoid VR_ANTI_RANGE
> >  by using range_int_cst_p.
> >  (format_integer): Likewise.
> >  (handle_printf_call): Guard against VR_ANTI_RANGE.
> >  * graphite-sese-to-poly.cc (add_param_constraints): Likewise.
> >  * tree-ssa-strlen.cc (set_strlen_range): Likewise.
> > ---
> >   gcc/gimple-ssa-sprintf.cc    | 6 +++---
> >   gcc/graphite-sese-to-poly.cc | 2 +-
> >   gcc/tree-ssa-strlen.cc       | 2 +-
> >   3 files changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/gcc/gimple-ssa-sprintf.cc b/gcc/gimple-ssa-sprintf.cc
> > index 18975708d2c..61974072f62 100644
> > --- a/gcc/gimple-ssa-sprintf.cc
> > +++ b/gcc/gimple-ssa-sprintf.cc
> > @@ -1082,7 +1082,7 @@ get_int_range (tree arg, gimple *stmt,
> >      value_range vr;
> >      query->range_of_expr (vr, arg, stmt);
> >   -	  if (!vr.undefined_p () && !vr.varying_p ())
> > +	  if (range_int_cst_p (&vr))
> >        {
> >          HOST_WIDE_INT type_min
> >   		= (TYPE_UNSIGNED (argtype)
> > @@ -1391,7 +1391,7 @@ format_integer (const directive &dir, tree arg,
> > pointer_query &ptr_qry)
> >         value_range vr;
> >         ptr_qry.rvals->range_of_expr (vr, arg, dir.info->callstmt);
> >   -      if (!vr.varying_p () && !vr.undefined_p ())
> > +      if (range_int_cst_p (&vr))
> >    {
> >      argmin = wide_int_to_tree (TREE_TYPE (arg), vr.lower_bound ());
> >      argmax = wide_int_to_tree (TREE_TYPE (arg), vr.upper_bound ());
> > @@ -4623,7 +4623,7 @@ handle_printf_call (gimple_stmt_iterator *gsi,
> > pointer_query &ptr_qry)
> >      value_range vr;
> >      ptr_qry.rvals->range_of_expr (vr, size, info.callstmt);
> >   -	  if (!vr.undefined_p ())
> > +	  if (!vr.undefined_p () && vr.kind () != VR_ANTI_RANGE)
> >        {
> >          tree type = TREE_TYPE (size);
> >          tree tmin = wide_int_to_tree (type, vr.lower_bound ());
> > diff --git a/gcc/graphite-sese-to-poly.cc b/gcc/graphite-sese-to-poly.cc
> > index fbe7667380a..b89262640ac 100644
> > --- a/gcc/graphite-sese-to-poly.cc
> > +++ b/gcc/graphite-sese-to-poly.cc
> > @@ -426,7 +426,7 @@ add_param_constraints (scop_p scop, graphite_dim_t p,
> > tree parameter)
> >   
> >     if (INTEGRAL_TYPE_P (type)
> >         && get_range_query (cfun)->range_of_expr (r, parameter)
> > -      && !r.undefined_p ())
> > +      && range_int_cst_p (&r))
> >       {
> >         min = r.lower_bound ();
> >         max = r.upper_bound ();
> > diff --git a/gcc/tree-ssa-strlen.cc b/gcc/tree-ssa-strlen.cc
> > index 7508c1768a5..e1230522564 100644
> > --- a/gcc/tree-ssa-strlen.cc
> > +++ b/gcc/tree-ssa-strlen.cc
> > @@ -1936,7 +1936,7 @@ set_strlen_range (tree lhs, wide_int min, wide_int
> > max,
> >    {
> >      value_range r;
> >      get_range_query (cfun)->range_of_expr (r, bound);
> > -	  if (!r.undefined_p ())
> > +	  if (range_int_cst_p (&r))
> >        {
> >          /* For a bound in a known range, adjust the range determined
> >      above as necessary.  For a bound in some anti-range or
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)