On Tue, Oct 9, 2018 at 6:23 PM Aldy Hernandez <al...@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;
> };
>
