Hi,
On Mon, 23 Jun 2014 19:12:47, Richard Biener wrote:
>
> On Mon, Jun 23, 2014 at 4:28 PM, Bernd Edlinger
> <bernd.edlin...@hotmail.de> wrote:
>> Hi,
>>
>> On Mon, 23 Jun 2014 10:40:53, Richard Biener wrote:
>>>
>>> On Sun, Jun 22, 2014 at 9:14 AM, Bernd Edlinger
>>> <bernd.edlin...@hotmail.de> wrote:
>>>> Hi,
>>>>
>>>> I noticed that several testcases in the GMP-4.3.2 test suite are failing
>>>> now which
>>>> did not happen with GCC 4.9.0. I debugged the first one, mpz/convert, and
>>>> found
>>>> the file mpn/generic/get_str.c was miscompiled.
>>>>
>>>> mpn/get_str.c.132t.dse2:
>>>> pretmp_183 = (sizetype) chars_per_limb_80;
>>>> pretmp_184 = -pretmp_183;
>>>> _23 = chars_per_limb_80 + 4294967295;
>>>> _68 = (sizetype) _23;
>>>> _28 = _68 + pretmp_184;
>>>>
>>>> mpn/get_str.c.133t.forwprop4:
>>>> _28 = 4294967295;
>>>>
>>>>
>>>> That is wrong, because chars_per_limb is unsigned, and it is not zero.
>>>> So the right result should be -1. This makes the loop termination in that
>>>> function fail.
>>>>
>>>> The reason for this is in this check-in:
>>>>
>>>> r210807 | ebotcazou | 2014-05-22 16:32:56 +0200 (Thu, 22 May 2014) | 3
>>>> lines
>>>>
>>>> * tree-ssa-forwprop.c (associate_plusminus): Extend (T)(P + A) - (T)P
>>>> -> (T)A transformation to integer types.
>>>>
>>>>
>>>> Because it implicitly assumes that integer overflow is not allowed with
>>>> all types,
>>>> including unsigned int.
>>>
>>> Hmm? But the transform is correct if overflow wraps. And it's correct if
>>> overflow is undefined as well, as (T)A is always well-defined
>>> (implementation
>>> defined) if it is a truncation.
>>>
>>
>> we have no problem when the cast from (P + A) to T is a truncation, except if
>> the add operation P + A is saturating.
>
> Ah, we indeed look at an inner operation.
>
>>> So we match the above an try to transform it to (T)P + (T)A - (T)P. That's
>>> wrong if the conversion is extending I think.
>>>
>>
>> Yes, in a way. But OTOH, Eric's test case opt37.adb, fails if we simply punt
>> here.
>>
>> Fortunately, with opt37.adb, P and A are signed 32-bit integers, and T is
>> size_t (64 bit)
>> and because the overflow of P + A causes undefined behaviour, we can assume
>> that
>> P + A _did_ not overflow, and therefore the transformation (T)(P + A) ==
>> (T)P + (T)A
>> is correct (but needs a strict overflow warning), and we still can use the
>> pattern
>> (T)P + (T)A - (T)P -> (T)A in this case.
>
> Ok, though that is then the only transform that uses strict-overflow
> semantics.
>
>> But we cannot use this transformation, as the attached test case demonstrates
>> when P + A is done in unsigned integers, because the result of the addition
>> is
>> different if it is done in unsigned int with allowed overflow, or in long
>> without
>> overflow.
>>
>>> Richard.
>>>
>>>
>>>>
>>>> The attached patch fixes these regressions, and because the reasoning
>>>> depends
>>>> on the TYPE_OVERFLOW_UNDEFINED attribute, a strict overflow warning has to
>>>> be
>>>> emitted here, at least for widening conversions.
>>>>
>>>>
>>>> Boot-strapped and regression-tested on x86_64-linux-gnu with all
>>>> languages, including Ada.
>>>> OK for trunk?
>>>
>>> + if (!TYPE_SATURATING (TREE_TYPE (a))
>>>
>>> this is already tested at the very beginning of the function.
>>>
>>
>> We have done TYPE_SATURATING (TREE_TYPE (rhs1)) that refers to T,
>> but I am concerned about the inner addition operation here,
>> and if it is done in a saturating way.
>
> Ok, a valid concern.
>
>>
>>> + && !FLOAT_TYPE_P (TREE_TYPE (a))
>>> + && !FIXED_POINT_TYPE_P (TREE_TYPE (a))
>>>
>>> likewise.
>>
>> Well, maybe this cannot happen, because if we have P + A, computed in float,
>> and T an integer type, then probably CONVERT_EXPR_CODE_P (def_code)
>> will not match, because def_code is FIX_TRUNC_EXPR in that case?
>
> Yes. It cannot be a FLOAT_TYPE_P here, similar for fixed-point which
> would use FIXED_CONVERT_EXPR. Saturating types don't seem to have
> a special conversion tree code.
>
then this should be an assertion, with a comment why this is supposed to be
impossible.
>> OTOH it does not hut to check that, becaue A's type may be quite different
>> than rhs1's type.
>>
>>>
>>> + || (!POINTER_TYPE_P (TREE_TYPE (p))
>>> + && INTEGRAL_TYPE_P (TREE_TYPE (a))
>>> + && TYPE_OVERFLOW_UNDEFINED (TREE_TYPE (a)))
>>>
>>> INTEGRAL_TYPE_P are always !POINTER_TYPE_P.
>>>
>>>
>>
>> We come here, either because P + A is a POINTER_PLUS_EXPR or because P + A
>> is a PLUS_EXPR.
>
> Ah, p vs. a - misread that code.
>
>> In the first case, P's type is POINTER_TYPE_P and A's type is INTEGRAL_TYPE_P
>> so this should not check the TYPE_OVERFLOW_UNDEFINED, but instead
>> the POINTER_TYPE_OVERFLOW_UNDEFINED.
>>
>> Also with undefined pointer wraparound, we can exploit that in the same way
>> as with
>> signed integers.
>>
>> But I am concerned, if (T)A is always the same thing as (T)(void*)A.
>> I'd say, yes, if TYPE_UNSIGNED (TREE_TYPE (p)) == TYPE_UNSIGNED (TREE_TYPE
>> (a))
>> or if A is a constant, and it is positive.
>
> I don't understand this last bit. If p is pointer then a is of
> unsigned sizetype.
> sizetypes size doesn't have to match pointer size.
>
Yes, maybe I was only a bit too shy here.
The resulting code (T)A casts directly from ptrdiff_t to T, while before the
transformation
the cast was from pointer_t to T. Is the result always the same, even for
negative A?
I was not sure, if A is always unsigned for all possible targets,
and if P is always unsigned and of same bit-size than A.
And if P may have different signedness than A what a pointer
wraparound means exactly.
If the answer is A and P are always unsigned and of same size,
then it would simplify this part significantly.
> Can you re-work the patch to split the case that doesn't exploit the undefined
> behavior (and thus does not need to warn) and the case that does,
> adding comments with reasoning before the individual || tests?
>
The first part will break gnat.dg/opt37.adb,
and the second part will fix it again, is that OK?
> The condition in your patch is unwiedingly large.
>
Sorry, I should have warned you, it caused myself severe headaches
when I wrote it down :-)
> Richard.
>
>>
>>
>> Thanks
>> Bernd.
>>
>>Then I dont need both conditions, but I was unsure here, because
