Patchwork [2/2] Enable elimination of zext/sext

login
register
mail settings
Submitter Kugan
Date Aug. 28, 2014, 3:45 a.m.
Message ID <53FEA5F0.9070906@linaro.org>
Download mbox | patch
Permalink /patch/383668/
State New
Headers show

Comments

Kugan - Aug. 28, 2014, 3:45 a.m.
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]

How about doing something like this to fix it.



If this looks reasonable I will do proper testing and post the results
with the Changelog.

Thanks,
Kugan
Marc Glisse - Aug. 28, 2014, 6:44 a.m.
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.
Kugan - Aug. 28, 2014, 7:29 a.m.
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

Patch

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);