On Thu, Oct 8, 2015 at 1:40 AM, Kugan <kugan.vivekanandara...@linaro.org> wrote:
>
>
> On 07/10/15 19:20, Richard Biener wrote:
>> On Wed, Oct 7, 2015 at 1:12 AM, kugan <kugan.vivekanandara...@linaro.org>
>> wrote:
>>>
>>> Hi Richard,
>>>
>>> Thanks for the review.
>>>
>>> On 15/09/15 23:08, Richard Biener wrote:
>>>>
>>>> On Mon, Sep 7, 2015 at 4:58 AM, Kugan <kugan.vivekanandara...@linaro.org>
>>>> wrote:
>>>>>
>>>>> This patch tree-vrp handling and optimization for ZEXT_EXPR.
>>>>
>>>>
>>>> + else if (code == SEXT_EXPR)
>>>> + {
>>>> + gcc_assert (range_int_cst_p (&vr1));
>>>> + unsigned int prec = tree_to_uhwi (vr1.min);
>>>> + type = vr0.type;
>>>> + wide_int tmin, tmax;
>>>> + wide_int type_min, type_max;
>>>> + wide_int may_be_nonzero, must_be_nonzero;
>>>> +
>>>> + gcc_assert (!TYPE_UNSIGNED (expr_type));
>>>>
>>>> hmm, I don't think we should restrict SEXT_EXPR this way. SEXT_EXPR
>>>> should operate on both signed and unsigned types and the result type
>>>> should be the same as the type of operand 0.
>>>>
>>>> + type_min = wi::shwi (1 << (prec - 1),
>>>> + TYPE_PRECISION (TREE_TYPE (vr0.min)));
>>>> + type_max = wi::shwi (((1 << (prec - 1)) - 1),
>>>> + TYPE_PRECISION (TREE_TYPE (vr0.max)));
>>>>
>>>> there is wi::min_value and max_value for this.
>>>
>>>
>>> As of now, SEXT_EXPR in gimple is of the form: x = y sext 8 and types of all
>>> the operand and results are of the wider type. Therefore we cant use the
>>> wi::min_value. Or do you want to convert this precision (in this case 8) to
>>> a type and use wi::min_value?
>>
>> I don't understand - wi::min/max_value get a precision and sign, not a type.
>> your 1 << (prec - 1) is even wrong for prec > 32 (it's an integer type
>> expression).
>> Thus
>>
>> type_min = wi::min_value (prec, SIGNED);
>> type_max = wi::max_value (prec, SIGNED);
>>
>
> Thanks for the comments. Is the attached patch looks better. It is based
> on the above. I am still assuming the position of sign-bit in SEXT_EXPR
> will be less than 64bit (for calculating sign_bit in wide_int format). I
> think this will always be the case but please let me know if this is not OK.
+ unsigned int prec = tree_to_uhwi (vr1.min);
this should use unsigned HOST_WIDE_INT
+ wide_int sign_bit = wi::shwi (1ULL << (prec - 1),
+ TYPE_PRECISION (TREE_TYPE (vr0.min)));
use wi::one (TYPE_PRECISION (TREE_TYPE (vr0.min))) << (prec - 1);
That is, you really need to handle precisions bigger than HOST_WIDE_INT.
But I suppose wide_int really misses a test_bit function (it has a set_bit
one already).
+ if (wi::bit_and (must_be_nonzero, sign_bit) == sign_bit)
+ {
+ /* If to-be-extended sign bit is one. */
+ tmin = type_min;
+ tmax = may_be_nonzero;
I think tmax should be zero-extended may_be_nonzero from prec.
+ else if (wi::bit_and (may_be_nonzero, sign_bit)
+ != sign_bit)
+ {
+ /* If to-be-extended sign bit is zero. */
+ tmin = must_be_nonzero;
+ tmax = may_be_nonzero;
likewise here tmin/tmax should be zero-extended may/must_be_nonzero from prec.
+ case SEXT_EXPR:
+ {
+ unsigned int prec = tree_to_uhwi (op1);
+ wide_int sign_bit = wi::shwi (1ULL << (prec - 1),
+ TYPE_PRECISION (TREE_TYPE (vr0.min)));
+ wide_int mask = wi::shwi (((1ULL << (prec - 1)) - 1),
+ TYPE_PRECISION (TREE_TYPE (vr0.max)));
this has the same host precision issues of 1ULL (HOST_WIDE_INT).
There is wi::mask, eventually you can use wi::set_bit_in_zero to
produce the sign-bit wide_int (also above).
The rest of the patch looks ok.
Richard.
> Thanks,
> Kugan
>
>
>>
>>> Please find the patch that addresses the other comments.
