Aldy Hernandez <al...@redhat.com> writes:
> On 10/14/19 4:30 AM, Richard Sandiford wrote:
>> Aldy Hernandez <al...@redhat.com> writes:
>>> On 10/11/19 10:42 AM, Richard Sandiford wrote:
>>>> The range-tracking code has a pretty hard-coded assumption that
>>>> is_gimple_min_invariant is equivalent to "INTEGER_CST or invariant
>>>> ADDR_EXPR". It seems better to add a predicate specifically for
>>>> that rather than contiually fight cases in which it can't handle
>>>> other invariants.
>>>
>>> I was going to suggest we normalize ranges to numerics completely before
>>> folding. That is, replacing normalize_addresses() here:
>>>
>>> *vr = op->fold_range (expr_type,
>>> vr0.normalize_addresses (),
>>> vr1.normalize_addresses ());
>>>
>>> ...into normalize_symbolics(). But I suppose getting the gate correct
>>> is even better. Thanks for taking the care of this extensive and manual
>>> change.
>>>
>>> The patch looks good to me. However, I do wonder if VRP and
>>> subsidiaries can't handle non-integer invariants, if we shouldn't
>>> disallow them from the setters as well:
>>>
>>> void
>>> value_range_base::set (tree val)
>>> {
>>> gcc_assert (TREE_CODE (val) == SSA_NAME || is_gimple_min_invariant
>>> (val));
>>> if (TREE_OVERFLOW_P (val))
>>> val = drop_tree_overflow (val);
>>> set (VR_RANGE, val, val);
>>> }
>>>
>>> void
>>> value_range::set (tree val)
>>> {
>>> gcc_assert (TREE_CODE (val) == SSA_NAME || is_gimple_min_invariant
>>> (val));
>>> if (TREE_OVERFLOW_P (val))
>>> val = drop_tree_overflow (val);
>>> set (VR_RANGE, val, val, NULL);
>>> }
>>>
>>> This would still allow setting of VARYING and UNDEFINED, but disallow
>>> poly-ints, etc from a range.
>>>
>>> Was this a small oversight, or was there a reason you left those in?
>>
>> Yeah, this was intentional. The patch is effectively treating
>> POLY_INY_CST as symbolic rather than constant. (It's really somewhere
>> in the middle: it's at least a function invariant, but the invariant
>> depends on a runtime target property.) So places like here that
>> can handle both symbolics and constants should be able to handle
>> POLY_INT_CST wihout problems. We just need to make sure that
>> POLY_INT_CSTs aren't treated as constants for range tracking,
>> because they're not "constant enough" there.
>
> Hmmm... We don't handle POLY_INT_CST value_range's anywhere, so perhaps
> it's better to stop their creation at the source and fix the caller, to
> inhibit their proliferation. AFAICT, the only POLY_INT_CST ranges that
> would be used are VARYING and UNDEFINED (for the lattice), and that
> already works.
>
> If you don't agree, then at least a comment in ::set() would be nice, to
> document that we're allowing their creation and why.
I don't think it makes sense to allow SSA_NAME (which is completely
unconstrained) and not allow POLY_INT_CST. Like I say, POLY_INT_CST
can be conservatively treated as symbolic.
POLY_INT_CST can produce useful singleton ranges or be a minimum or
a maximum, even if we don't use them to reduce ranges arithmetically.
Richard
