Re: [PATCH] Fix forwporp pattern (T)(P + A) - (T)P -> (T)A
> 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
On Mon, Jun 23, 2014 at 8:02 PM, Bernd Edlinger wrote: > Hi, > > On Mon, 23 Jun 2014 19:12:47, Richard Biener wrote: >> >> On Mon, Jun 23, 2014 at 4:28 PM, Bernd Edlinger >> wrote: >>> Hi, >>> >>> On Mon, 23 Jun 2014 10:40:53, Richard Biener wrote: On Sun, Jun 22, 2014 at 9:14 AM, 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. > > 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 >>> th
RE: [PATCH] Fix forwporp pattern (T)(P + A) - (T)P -> (T)A
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
RE: [PATCH] Fix forwporp pattern (T)(P + A) - (T)P -> (T)A
Hi, On Mon, 23 Jun 2014 19:12:47, Richard Biener wrote: > > On Mon, Jun 23, 2014 at 4:28 PM, Bernd Edlinger > wrote: >> Hi, >> >> On Mon, 23 Jun 2014 10:40:53, Richard Biener wrote: >>> >>> On Sun, Jun 22, 2014 at 9:14 AM, 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. 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 i
RE: [PATCH] Fix forwporp pattern (T)(P + A) - (T)P -> (T)A
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
On Mon, Jun 23, 2014 at 4:28 PM, Bernd Edlinger wrote: > Hi, > > On Mon, 23 Jun 2014 10:40:53, Richard Biener wrote: >> >> On Sun, Jun 22, 2014 at 9:14 AM, 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. >>> >>> 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 reasoni
Re: [PATCH] Fix forwporp pattern (T)(P + A) - (T)P -> (T)A
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
Hi, On Mon, 23 Jun 2014 10:40:53, Richard Biener wrote: > > On Sun, Jun 22, 2014 at 9:14 AM, 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. >> >> 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
> 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
On Sun, Jun 22, 2014 at 9:14 AM, 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. > > 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. >
[PATCH] Fix forwporp pattern (T)(P + A) - (T)P -> (T)A
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. 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? Thanks Bernd. gcc/ChangeLog: 2014-06-22 Bernd Edlinger * tree-ssa-forwprop.c (associate_plusminus): For widening conversions check for undefined overflow in (T)(P + A) - (T)P -> (T)A. Issue a strict overflow warning if appropriate. testsuite/ChangeLog: 2014-06-22 Bernd Edlinger * gcc.c-torture/execute/20140622-1.c: New test. patch-forwprop.diff Description: Binary data
