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

2014-06-26 Thread Eric Botcazou
 Only if we could somehow rule out that chars_per_limb can be zero.
 Then we know for sure that unsigned overflow must happen, and
 the only possible result would be -1.
 But at this time, both -1 and 4294967295 are possible.

I see, I thought you meant that the result was -1 statically.

Thanks for correcting this annoying blunder...

-- 
Eric Botcazou


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

2014-06-24 Thread Richard Biener
On Mon, Jun 23, 2014 at 8:02 PM, Bernd Edlinger
bernd.edlin...@hotmail.de wrote:
 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.

Hum, well. If then such check belongs in the gimple verifier in tree-cfg.c, not
spread (and duplicated) in random code.

 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.


 

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

2014-06-23 Thread Richard Biener
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.

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.

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.

+  !FLOAT_TYPE_P (TREE_TYPE (a))
+  !FIXED_POINT_TYPE_P (TREE_TYPE (a))

likewise.

+ || (!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.







 Thanks
 Bernd.



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

2014-06-23 Thread Eric Botcazou
 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.

Can't we compute the right result in this case?  4294967295 is almost -1.

 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.

Saturating, floating-point and fixed-point types are already excluded here, 
see the beginning of the function. 

-- 
Eric Botcazou


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

2014-06-23 Thread 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.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.
 
 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.

  

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

2014-06-23 Thread Jeff Law

On 06/22/14 01:14, Bernd Edlinger 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.
It's interesting you stumbled on this.  I've seen this issue come and go 
as well in some of my integration testing work with the trunk.



Jeff


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

2014-06-23 Thread Richard Biener
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.

 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.

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 condition in your patch is unwiedingly large.

Richard.



 Thanks
 Bernd.




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

2014-06-23 Thread Bernd Edlinger
Hi,

On Mon, 23 Jun 2014 10:27:59, Jeff Law wrote:

 On 06/22/14 01:14, Bernd Edlinger 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.
 It's interesting you stumbled on this. I've seen this issue come and go
 as well in some of my integration testing work with the trunk.


 Jeff


Yes, it will likely come with -m64 and go with -m32.


Bernd.
  

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

2014-06-23 Thread Bernd Edlinger
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 

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

2014-06-23 Thread Bernd Edlinger
Hi,

On Mon, 23 Jun 2014 16:22:13, Eric Botcazou wrote:

 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.

 Can't we compute the right result in this case? 4294967295 is almost -1.


Only if we could somehow rule out that chars_per_limb can be zero.
Then we know for sure that unsigned overflow must happen, and
the only possible result would be -1. 
But at this time, both -1 and 4294967295 are possible.

What happened before, is this:

  int i;
  unsigned int chars_per_limb;

  i_79 = __gmpn_bases[base_30(D)].chars_per_limb;
  chars_per_limb_80 = (unsigned int) i_79;

so how can we know that chars_per_limb is 0, here?

In this example probably not, only maybe if someone helps
us and adds an assertion (chars_per_limb 0); then we could
of course exploit that information



Bernd.

 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.

 Saturating, floating-point and fixed-point types are already excluded here,
 see the beginning of the function.

 --
 Eric Botcazou