Aldy Hernandez <al...@redhat.com> writes:
> On 11/10/20 3:35 PM, Richard Sandiford wrote:
>> Aldy Hernandez <al...@redhat.com> writes:
>>>> (actually I can see 3245 ICEs on aarch64)
>>>>
>>>> Can you fix it?
>>>
>>> Sure can.
>>>
>>> Richard, I seem to have incorrectly removed the early exit for varying,
>>> and that affected the changes you made for poly ints. Is there any
>>> reason we can't just exit and set varying without checking for kind !=
>>> VR_VARYING?
>>
>> No reason, it seemed more natural to drop to a lower kind with the old
>> code :-) (But not with the new code.)
>>
>> But it isn't obvious to me why the code is now structed the way it is.
>>
>> if (POLY_INT_CST_P (min) || POLY_INT_CST_P (max))
>> {
>> set_varying (TREE_TYPE (min));
>> return;
>> }
>>
>> // Nothing to canonicalize for symbolic ranges.
>> if (TREE_CODE (min) != INTEGER_CST
>> || TREE_CODE (max) != INTEGER_CST)
>> {
>> m_kind = kind;
>> m_base[0] = min;
>> m_base[1] = max;
>> m_num_ranges = 1;
>> return;
>> }
>>
>> swap_out_of_order_endpoints (min, max, kind);
>> if (kind == VR_VARYING)
>> {
>> set_varying (TREE_TYPE (min));
>> return;
>> }
>>
>> Why do we want to check “min” and “max” being INTEGER_CST before “kind”
>> being VR_VARYING, and the potentially record VR_VARYING with specific
>> bounds? And why do we want to swap the “min” and “max” before checking
>> whether “kind” is VR_VARYING (when we'll then drop the min and max anyway)?
>> I think this would benefit from a bit more commentary at least.
>
> The main idea was to shorten the code and avoid having to exit due to
> varying at various points (early and after the operands had been
> swapped). But yes, it took more cycles.
>
> BTW, VR_VARYING does get specific bounds, by design. What could've
> happened in the code was someone feeding VR_VARYING with non-integer
> bounds. This would've built an invalid VR_VARYING.
>
> How about this (on top of the previous patch which I already pushed to
> un-break aarch64)?
Thanks, this certainly makes the flow clearer for a range noob like me :-)
> p.s. If POLY_INT_CST_P are not supported in ranges, but are
> INTEGRAL_TYPE_P, perhaps we should also tweak irange::supports_type_p so
> it doesn't leak in anywhere else.
POLY_INT_CSTs aren't associated with separate types. They're just
values of normal integer type. Logically they come somewhere between an
INTEGER_CST and an SSA_NAME: they're not “as constant as” an INTEGER_CST
but not “as variable as” an SSA_NAME.
This means that they really could be treated as symbolic. The difficulty
is that POLY_INT_CSTs satisfy is_gimple_min_invariant, and the VRP code
fundamentally assumes that is_gimple_min_invariant on an integer type
means that the value must be an INTEGER_CST or an ADDR_EXPR. At one
point I'd posted a patch to add a new predicate specifically for that,
but Richard's response was that noone would remember to use it (which is
a fair comment :-)). So the current approach is instead to stop
POLY_INT_CSTs getting into the VRP system in the first place.
If the VRP code was rigorous about checking for INTEGER_CST before assuming
that something was an INTEGER_CST then no special handling of POLY_INT_CST
would be needed. But that's not the style that GCC generally follows and
trying to retrofit it now seems like fighting against the tide.
> diff --git a/gcc/value-range.cc b/gcc/value-range.cc
> index b7ccba010e4..3703519b03a 100644
> --- a/gcc/value-range.cc
> +++ b/gcc/value-range.cc
> @@ -249,7 +249,8 @@ irange::set (tree min, tree max, value_range_kind kind)
> return;
> }
>
> - if (POLY_INT_CST_P (min) || POLY_INT_CST_P (max))
> + if (kind == VR_VARYING
> + || POLY_INT_CST_P (min) || POLY_INT_CST_P (max))
> {
> set_varying (TREE_TYPE (min));
> return;
Very minor nit, sorry, but: formatting rules say that all checks should
be on one line or that there should be exactly one check per line.
OK with that change. Thanks for the quick response in fixing this.
Richard
