Martin Sebor <mse...@gmail.com> writes:
> On 02/24/2018 02:32 AM, Richard Sandiford wrote:
>> Martin Sebor <mse...@gmail.com> writes:
>>> On 02/23/2018 01:13 PM, Jakub Jelinek wrote:
>>>> On Fri, Feb 23, 2018 at 12:57:14PM -0700, Martin Sebor wrote:
>>>>> + /* get_inner_reference is not expected to return null. */
>>>>> + gcc_assert (base != NULL);
>>>>> +
>>>>> poly_int64 bytepos = exact_div (bitpos, BITS_PER_UNIT);
>>>>>
>>>>> - HOST_WIDE_INT const_off;
>>>>> - if (!base || !bytepos.is_constant (&const_off))
>>>>> - {
>>>>> - base = get_base_address (TREE_OPERAND (expr, 0));
>>>>> - return;
>>>>> - }
>>>>> -
>>>>> + /* There is no conversion from poly_int64 to offset_int even
>>>>> + though the latter is wider, so go through HOST_WIDE_INT.
>>>>> + The offset is expected to always be constant. */
>>>>> + HOST_WIDE_INT const_off = bytepos.to_constant ();
>>>>
>>>> The assert is ok, but removing the bytepos.is_constant (&const_off)
>>>> is wrong, I'm sure Richard S. can come up with some SVE testcase
>>>> where it will not be constant. If it is not constant, you can handle
>>>> it like var_off (which as I said on IRC or in the PR also seems to be
>>>> incorrect, because if the base is not a decl the variable offset could be
>>>> negative).
>>>
>>> That's what I did at first. I took it out because it sounded
>>> to me like you were saying it was a hypothetical possibility,
>>> not something that would ever happen in practice, and because
>>> I have no way of testing that code. I have put it back in
>>> the attached update.
>>>
>>> Since you added Richard: can you please explain how to convert
>>> a poly64_int to offset_int without converting it to HOST_WIDE_INT,
>>> or if it can't be done, why not? It's cumbersome and error-prone
>>> to have to go through HWI, and doing so defeats the main goal of
>>> using offset_int in the gimple-ssa-warn-restrict pass. Should
>>> the pass (and others like it) be changed to store offsets and
>>> sizes in some flavor of poly_int instead of offset_int?
>>
>> Not sure if this is what you mean, but:
>>
>> bool f (poly_int64 x, offset_int *y) { return x.is_constant (y); }
>>
>> does work. "y" doesn't have to be "HOST_WIDE_INT *".
>
> Great, that works, thank you!
>
> Can you also comment on whether the poly64_int offset returned
> by get_inner_reference() can be non-constant? O'm trying to
> understand why/how that could happen and if there is a way to
> test it in the -Wrestrict pass (i.e., come up with a test case
> involving memcpy or similar built-in).
Yeah, get_inner_reference can return non-constant offsets (but so far
only on SVE of course). E.g. the SVE versions of IFN_LOAD/STORE_LANES
use arrays of variable-length vectors, so vectors 1 and above will be at
non-constant offsets. Also, the OpenMP support creates vector-sized
variables, and we can have references to the last element of such
a vector.
I don't think it can yet happen for cases that are interesting to the
-Wrestrict warning though, so dropping back to conservatively correct
behaviour (via is_constant) is fine.
And that's the general principle: if one of the inputs has poly type,
try to use poly types for the whole operation if it's easy to do,
otherwise drop to conservatively correct behaviour using is_constant.
Only use to_constant if there is a known reason for it to be correct
(rather than no known reason for it to be incorrect).
Thanks,
Richard
