| Message ID | 53FEA5F0.9070906@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Thu, 28 Aug 2014, Kugan wrote: > On 27/08/14 23:02, Kugan wrote: >> On 27/08/14 20:01, Uros Bizjak wrote: >>> Hello! >>> >>>> 2014-08-07 Kugan Vivekanandarajah <kuganv@linaro.org> >>>> >>>> * calls.c (precompute_arguments): Check >>>> promoted_for_signed_and_unsigned_p and set the promoted mode. >>>> (promoted_for_signed_and_unsigned_p): New function. >>>> (expand_expr_real_1): Check promoted_for_signed_and_unsigned_p >>>> and set the promoted mode. >>>> * expr.h (promoted_for_signed_and_unsigned_p): New function definition. >>>> * cfgexpand.c (expand_gimple_stmt_1): Call emit_move_insn if >>>> SUBREG is promoted with SRP_SIGNED_AND_UNSIGNED. >>> >>> This patch regresses: >>> >>> Running target unix >>> FAIL: libgomp.fortran/simd7.f90 -O2 execution test >>> FAIL: libgomp.fortran/simd7.f90 -Os execution test >>> >> >> [snip] >> >>> When compiling this code, we have: >>> >>> lhs = _63 >>> target = (subreg/s/v/u:SI (reg:DI 145 [ D.1694 ]) 0) >>> temp = (subreg:SI (reg:DI 540) 0) >>> >>> So, the code assumes that it is possible to copy (reg:DI 540) directly >>> to (reg:DI 154). However, this is not the case, since we still have >>> garbage in the top 32bits. >>> >>> Reverting the part above fixes the runtime failure, since (insn 599) is now: >>> >>> (insn 599 598 0 (set (reg:DI 145 [ D.1694 ]) >>> (zero_extend:DI (subreg:SI (reg:DI 540) 0))) -1 >>> (nil)) >>> >>> It looks to me that we have also to check the temp with SUBREG_PROMOTED_*. >> >> Sorry for the breakage. I am looking into this now and I can reproduce >> it on qemu-alpha. >> >> I have noticed the following VRP data which is used in deciding this >> erroneous removal. It seems suspicious to me. >> >> _343: [2147483652, 2147483715] >> _344: [8, 134] >> _345: [8, 134] >> >> _343 = ivtmp.179_52 + 2147483645; >> _344 = _343 * 2; >> _345 = (integer(kind=4)) _344; >> >> Error comes from the third statement. > > In tree-vrp.c, in extract_range_from_binary_expr_1, there is a loss of > precision and the value_range is truncated. For the test-case provided > by Uros, it is > > _344 = _343 * 2; > [...,0x100000008], precision = 384 > [...,0x100000086], precision = 384 > > and it is converted to following when it goes from wide_int to tree. > [8, 134] Why do you believe that is wrong? Assuming _344 has a 32 bit type with wrapping overflow, this is just doing the wrapping modulo 2^32.
On 28/08/14 16:44, Marc Glisse wrote: > On Thu, 28 Aug 2014, Kugan wrote: > >> On 27/08/14 23:02, Kugan wrote: >>> On 27/08/14 20:01, Uros Bizjak wrote: >>>> Hello! >>>> >>>>> 2014-08-07 Kugan Vivekanandarajah <kuganv@linaro.org> >>>>> >>>>> * calls.c (precompute_arguments): Check >>>>> promoted_for_signed_and_unsigned_p and set the promoted mode. >>>>> (promoted_for_signed_and_unsigned_p): New function. >>>>> (expand_expr_real_1): Check promoted_for_signed_and_unsigned_p >>>>> and set the promoted mode. >>>>> * expr.h (promoted_for_signed_and_unsigned_p): New function >>>>> definition. >>>>> * cfgexpand.c (expand_gimple_stmt_1): Call emit_move_insn if >>>>> SUBREG is promoted with SRP_SIGNED_AND_UNSIGNED. >>>> >>>> This patch regresses: >>>> >>>> Running target unix >>>> FAIL: libgomp.fortran/simd7.f90 -O2 execution test >>>> FAIL: libgomp.fortran/simd7.f90 -Os execution test >>>> >>> >>> [snip] >>> >>>> When compiling this code, we have: >>>> >>>> lhs = _63 >>>> target = (subreg/s/v/u:SI (reg:DI 145 [ D.1694 ]) 0) >>>> temp = (subreg:SI (reg:DI 540) 0) >>>> >>>> So, the code assumes that it is possible to copy (reg:DI 540) directly >>>> to (reg:DI 154). However, this is not the case, since we still have >>>> garbage in the top 32bits. >>>> >>>> Reverting the part above fixes the runtime failure, since (insn 599) >>>> is now: >>>> >>>> (insn 599 598 0 (set (reg:DI 145 [ D.1694 ]) >>>> (zero_extend:DI (subreg:SI (reg:DI 540) 0))) -1 >>>> (nil)) >>>> >>>> It looks to me that we have also to check the temp with >>>> SUBREG_PROMOTED_*. >>> >>> Sorry for the breakage. I am looking into this now and I can reproduce >>> it on qemu-alpha. >>> >>> I have noticed the following VRP data which is used in deciding this >>> erroneous removal. It seems suspicious to me. >>> >>> _343: [2147483652, 2147483715] >>> _344: [8, 134] >>> _345: [8, 134] >>> >>> _343 = ivtmp.179_52 + 2147483645; >>> _344 = _343 * 2; >>> _345 = (integer(kind=4)) _344; >>> >>> Error comes from the third statement. >> >> In tree-vrp.c, in extract_range_from_binary_expr_1, there is a loss of >> precision and the value_range is truncated. For the test-case provided >> by Uros, it is >> >> _344 = _343 * 2; >> [...,0x100000008], precision = 384 >> [...,0x100000086], precision = 384 >> >> and it is converted to following when it goes from wide_int to tree. >> [8, 134] > > Why do you believe that is wrong? Assuming _344 has a 32 bit type with > wrapping overflow, this is just doing the wrapping modulo 2^32. > Indeed. I missed the TYPE_OVERFLOW_WRAPS check earlier. Thanks for pointing me to that. Kugan
diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c index d16fd8a..c0fb902 100644 --- a/gcc/tree-vrp.c +++ b/gcc/tree-vrp.c @@ -2625,6 +2625,8 @@ extract_range_from_binary_expr_1 (value_range_t *vr, <wi::extended_tree <WIDE_INT_MAX_PRECISION * 2> > vrp_int_cst; vrp_int sizem1 = wi::mask <vrp_int> (prec, false); vrp_int size = sizem1 + 1; + vrp_int type_min = vrp_int_cst (TYPE_MIN_VALUE (expr_type)); + vrp_int type_max = vrp_int_cst (TYPE_MAX_VALUE (expr_type)); /* Extend the values using the sign of the result to PREC2. From here on out, everthing is just signed math no matter @@ -2688,7 +2690,9 @@ extract_range_from_binary_expr_1 (value_range_t *vr, /* diff = max - min. */ prod2 = prod3 - prod0; - if (wi::geu_p (prod2, sizem1)) + if (wi::geu_p (prod2, sizem1) + || wi::lts_p (prod0, type_min) + || wi::gts_p (prod3, type_max)) { /* the range covers all values. */ set_value_range_to_varying (vr);