On 10/14/19 8:31 AM, Richard Sandiford wrote:
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.
Oh, I see your change to ::symbolic_p() would effectively treat them as
symbolics.
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.
In which case, singleton_p() should be adapted to return the singleton.
I don't expect you to do this, unless you want to :). Could you at
least put a comment there, so we don't forget?
I'm feel a bit queasy allowing POLY_INT ranges throughout, when we have
no code that handle them anywhere, but I won't object to your patch.
Thanks for fixing this.
The patch with the comment on singleton is fine with me.
Aldy
