Re: [range-ops] patch 01/04: types for VR_UNDEFINED and VR_VARYING

[Richard Biener <richard dot guenther at gmail dot com>]

On Mon, Jul 1, 2019 at 10:52 AM Aldy Hernandez <aldyh@redhat.com> wrote:
>
> As discussed before, this enforces types on undefined and varying, which
> makes everything more regular, and removes some special casing
> throughout range handling.

I don't like it too much given you need to introduce that "cache".

Why do VARYING or UNDEFINED need a type?  Nobody should ever need
to ask for the type of a range anyhow - the type should be always that from
the context we're looking at (the SSA name the range is associated with,
the operation we are performing on one or two ranges, etc.).

Thinking again about this it looks fundamentally wrong to associate
a type with the VARYING or UNDEFINED lattice states.

Oh, and no, IPA shouldn't create ranges for floats - probably a sanity check
missing somewhere.

Richard.

> The min/max fields will contain TYPE_MIN_VALUE and TYPE_MAX_VALUE, which
> will make it easy to get at the bounds of a range later on.  Since
> pointers don't have TYPE_MIN/MAX_VALUE, we are using build_zero_cst()
> and wide_int_to_tree(wi::max_value(precision)), for consistency.
> UNDEFINED is set similarly, though nobody should ever ask for anything
> except type() from it.  That is, no one should be asking for the bounds.
>
> There is one wrinkle, ipa-cp creates VR_VARYING ranges of floats,
> presumably to pass around state??  This causes value_range_base::type()
> and others to fail, even though I haven't actually seen a case of
> someone actually trying to set a VR_RANGE of a float.  For now, I've
> built a NOP_EXPR wrapper so type() works correctly.  The correct
> approach would probably be to avoid creating these varying/undefined
> ranges in ipa-cp, but I don't have enough ipa-cp-foo to do so.
> Suggestions welcome, if you don't like special casing this for ipa-cp.
> Or perhaps as a follow up.
>
> In this patch I start introducing a couple small API changes that will
> be needed for range-ops.  Since they're needed/useful for this patch, I
> started adding them on a need-to-use basis.  They are:
>
> value_range_base (tree type)
>
>         This is our constructor for building a varying:
>                 value_range_base foo (some_type);
>
> value_range_base::supports_type_p(tree type)
>
>         We use this instead of having to test everywhere for
>         INTEGRAL_TYPE_P and POINTER_TYPE_P which VRP uses throughout.
>         I have not ported every use of the INTEGRAL || POINTER in the
>         compiler to this function.  But that could be a follow up
>         cleanup if someone (else) is interested :).
>
> value_range_base_normalize_symbolics():
>
>         This normalizes symbolics into constants.  In VRP we usually
>         normalize necessary symbolics into [MIN, MAX].  This patch does
>         slightly better.  Now we transform:
>
>           // [SYM, SYM] -> VARYING
>           // [SYM, NUM] -> [-MIN, NUM]
>           // [NUM, SYM] -> [NUM, +MAX]
>           // ~[SYM, NUM] -> [NUM + 1, +MAX]
>           // ~[NUM, SYM] -> [-MIN, NUM - 1]
>
>         TBH, this bit and its use in *multiplicative_op probably fits
>         better with the canonicalization patch, but as I said.  They're
>         a bit intertwined.  Ughh.
>
> Finally, as you mentioned before, we need a way of caching varyings in
> the allocation pool.  The type_range_cache class in the attached patch
> is Andrew's doing, but I'll be happy to take the blame and address
> anything that needs doing.
>
> Tested on x86-64 Linux with --enable-languages=all.
>
> Aldy