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 <rguent...@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)
