Re: Add a constant_range_value_p function (PR 92033)

[Richard Sandiford <richard.sandiford@arm.com>]

Aldy Hernandez <aldyh@redhat.com> writes:
> On 10/14/19 4:30 AM, Richard Sandiford wrote:
>> Aldy Hernandez <aldyh@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