Re: [patch] new API for value_range

[Richard Biener <richard.guenther@gmail.com>]

On Tue, Oct 9, 2018 at 6:23 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>
> I'm assuming the silence on the RFC means nobody is viscerally opposed
> to it, so here goes the actual implementation ;-).
>
>         FWI: https://gcc.gnu.org/ml/gcc-patches/2018-10/msg00157.html
>
> My aim is no change to the current functionality, but there are some
> things that changed slightly (with no appreciable change in
> bootstrapability or tests).
>
> 1.  Primarily, we were building value_ranges by modifying them in-flight
> with no regards to the validity of the resulting range.  By enforcing
> the API, I noticed we periodically built VR_VARYING / VR_UNDEFINED, but
> left the equivalence bits uncleared.  This comment in the original
> header file indicates that this is invalid behavior:
>
>    /* Set of SSA names whose value ranges are equivalent to this one.
>       This set is only valid when TYPE is VR_RANGE or VR_ANTI_RANGE.  */
>
> The API now enforces this upon construction.
>
> 2. I also saw us setting min/max when VARYING or UNDEFINED was set.
> This is invalid.  Although these values were being ignored, the API now
> enforces this.
>
> 3. I saw one case in set_value_range_with_overflow() were we were
> building an invalid range with swapped ranges, where we were silently
> depending on somebody further up the call chain to swap them for us.
> I've fixed this at creation.
>
> 4. There is one assert in ipcp_vr_lattice which I hope to remove, but
> left as proof that the original VR_UNDEFINED set was not necessary, as
> it is now done by default on an empty constructor:
>
> -  void init () { m_vr.type = VR_UNDEFINED; }
> +  void init () { gcc_assert (m_vr.undefined_p ()); }
>
> One last note.  The file tree-vrp.c already has a cripple API of sorts
> in the form of functions (set_value_range_to_varying, etc).  I have
> tried to keep those functions available, by calling the API under the
> covers, but would be okay in removing them altogether as a follow-up.
>
> Please refer to the RFC wrt the min/max/vrtype accessors, as well as the
> new tree type field.
>
> I am quoting the class declaration below to make it easy to review at a
> high level.
>
> Tested on x86-64 Linux.  All languages, including Ada and Go.
>
> OK for trunk?

Reviewing in patch order.

> Aldy
>
> class GTY((for_user)) value_range
> {
>   public:
>    value_range ();
>    value_range (tree type);
>    value_range (value_range_type, tree type, tree, tree, bitmap = NULL);
>    bool operator== (const value_range &) const;
>    bool operator!= (const value_range &) const;
>    void intersect (const value_range *);
>    void union_ (const value_range *);

with trailing underscore?  seriously?

>    /* Like operator== but ignore equivalence bitmap.  */
>    bool ignore_equivs_equal_p (const value_range &) const;
>    /* Like a operator= but update equivalence bitmap efficiently.  */
>    void copy_with_equiv_update (const value_range *);
>
>    /* Types of value ranges.  */
>    bool undefined_p () const;
>    bool varying_p () const;
>    bool symbolic_p () const;
>    bool numeric_p () const;
>    void set_undefined (tree = NULL);
>    void set_varying (tree = NULL);

I'd appreciate comments on those predicates, esp. as you
replace positive tests by negative ones like in

   /* If we found any usable VR, set the VR to ssa_name and create a
      PUSH old value in the stack with the old VR.  */
-  if (vr.type == VR_RANGE || vr.type == VR_ANTI_RANGE)
+  if (!vr.undefined_p () && !vr.varying_p ())
     {

I'd also spell numeric_p as constant_p or drop it alltogether
since !symbolic_p should imply it given varying_p and undefined_p
are just some special-cases of "numeric_p" (full and empty range).

That said, for the time being I'd use non_symbolic_range_or_anti_range_p
instead of numeric_p () (seeing that you maybe want to hide the fact
that we have anti-ranges?)

-  value_range vr = VR_INITIALIZER;
+  value_range vr (TREE_TYPE (name));

so you basically forgo with the fact that empty ranges are universal?
I don't like it too much that we have to invent a type here.  Why enforce this
and not allow/force type == NULL_TREE for empty ranges?

One could argue VARYING is also universal to some extent and useful
only with context, so similar argument applies to your change forcing
a type for set_value_range_to_varying.

-      value_range vr = VR_INITIALIZER;
+      value_range vr;

oh, so you do have a default constructor.

>
>    /* Equivalence bitmap methods.  */
>    bitmap equiv () const;
>    void set_equiv (bitmap);

Err, I think we've settled on _not_ wrapping all member accesses
with get/set methods, didn't we?  I personally dislike that very much.

>    void equiv_free ();
>    void equiv_copy (const value_range *);
>    void equiv_clear ();
>    void equiv_and (const value_range *);
>    void equiv_ior (const value_range *);

Likewise I find this useless abstraction.  It's even questionable
if _free/_clear/_copy are good APIs here.  This should be all
hidden in intersect/union which I do not find in the API at all...

>
>    /* Misc methods.  */
>    tree type () const;

type() and vrtype() is confusing - value_type() and range_kind() maybe?

>    bool null_p () const;
>    bool may_contain_p (tree) const;
>    tree singleton () const;

No documentation? :/   Why null_p but singleton (instead of singleton_p)?

>    void set_and_canonicalize (enum value_range_type, tree, tree, tree,
> bitmap);

Why's that necessary if you enforce sanity?

>    void dump () const;
>
>    /* Temporary accessors that should eventually be removed.  */
>    enum value_range_type vrtype () const;
>    tree min () const;
>    tree max () const;
>
>   private:
>    void set (value_range_type, tree type, tree, tree, bitmap);
>    void check ();
>    bool equal_p (const value_range &, bool ignore_equivs) const;
>
>    enum value_range_type m_vrtype;
>   public:
>    /* These should be private, but GTY is a piece of crap.  */
>    tree m_min;
>    tree m_max;
>    tree m_type;

m_type is redundant (see above).

>    /* Set of SSA names whose value ranges are equivalent to this one.
>       This set is only valid when TYPE is VR_RANGE or VR_ANTI_RANGE.  */
>    bitmap m_equiv;
> };



>