Re: [PATCH] Tree-level fix for PR 69526

[Richard Biener <richard dot guenther at gmail dot com>]

On Mon, Nov 28, 2016 at 2:26 PM, Robin Dapp <rdapp@linux.vnet.ibm.com> wrote:
>>> +             /* Sign-extend @1 to TYPE.  */
>>> +             w1 = w1.from (w1, TYPE_PRECISION (type), SIGNED);
>>>
>>> not sure why you do always sign-extend.  If the inner op is unsigned
>>> and we widen then that's certainly bogus considering your UINT_MAX
>>> example above.  Does
>>>
>>>                w1 = w1.from (w1, TYPE_PRECISION (type), TYPE_SIGN (inner_type));
>>>
>>> not work for some reason?
>
> With TYPE_SIGN (inner_type), I encountered situations like this in the
> testsuite (mostly Fortran but also 20000422-1.c):
>
>   Folding statement: _1 = _9 + 1;
>   Applying pattern match.pd:1214, gimple-match.c:8719
>   gimple_simplified to _93 = (sizetype) _8;
>   _1 = _93 + 4294967296;
>   Folded into: _1 = _93 + 4294967296;
>
> in
>   _8 = (unsigned int) i_73;
>   _5 = _8 + 4294967295;
>   _9 = (sizetype) _5;
>   _93 = (sizetype) _8;
>   _1 = _93 + 4294967296;
>
> TYPE_SIGN (inner_type) is PLUS here, although it should be MINUS in
> order to combine -1 and +1 to 0. Perhaps this can be handled differently
> with keeping TYPE_SIGN (inner_type)?

So we have (uint64_t)(uint32 + -1U) + 1 and using TYPE_SIGN (inner_type)
produces (uint64_t)uint32 + -1U + 1.  This simply means that we cannot ignore
overflow of the inner operation and for some reason your change
to extract_range_from_binary_expr didn't catch this.  That is _8 + 4294967295
overflows but we ignored that.

> On the other hand, using SIGNED instead of TYPE_SIGN (inner_type) worked
> for all cases I looked at, like
>   if (a > 1 && a < 10)
>     return (unsigned long)(a + UINT_MAX) + 1;
> For
>   if (a > 0 && a < 10)
> extract_range...() would not find a non-VR_VARYING range although we
> should probably still be able to simplify this.
>
>   if (a > 0)
> Here, vrp figures out [0,4294967294] and the simplification takes place.
>
> For
>   if (a <= 10)
>     return (unsigned long)(a + (UINT_MAX - 10)) + 1;
> 003t.original already reads
>     return (long unsigned int) a + 4294967286
>
> From this I hand-wavingly deduced, we'd only see instances where a
> sign-extension of the constant is fine (test suites on s390x and x86
> affirm this for what it's woth) but I don't have a cogent reason hence
> my doubts :)

Well, even if sign-extending you can probably construct a testcase where
that's still wrong with respect to overflow.

Richard.

> I'm ok with omitting the sign-changing case (I hadn't though of it
> anyway) and adapted the patch. I haven't attached the updated version
> though, because I'm still unsure about the issue above (despite the
> clean test suite).
>
> Regards
>  Robin
>