[PATCH] Fix forwporp pattern (T)(P + A) - (T)P -> (T)A

[Bernd Edlinger]

Hi,

On Mon, 23 Jun 2014 10:40:53, Richard Biener wrote:
>
> On Sun, Jun 22, 2014 at 9:14 AM, Bernd Edlinger
> <bernd.edlinger@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.
 
> 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.

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.


> + && !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?

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.

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.



Thanks
Bernd.