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);
?
> Please find the patch that addresses the other comments.
I'll have a look later.
> Thanks,
> Kugan
>
>
>>
>> + HOST_WIDE_INT int_may_be_nonzero = may_be_nonzero.to_uhwi ();
>> + HOST_WIDE_INT int_must_be_nonzero = must_be_nonzero.to_uhwi ();
>>
>> this doesn't need to fit a HOST_WIDE_INT, please use wi::bit_and (can't
>> find a test_bit with a quick search).
>>
>> + tmin = wi::sext (tmin, prec - 1);
>> + tmax = wi::sext (tmax, prec - 1);
>> + min = wide_int_to_tree (expr_type, tmin);
>> + max = wide_int_to_tree (expr_type, tmax);
>>
>> not sure why you need the extra sign-extensions here.
>>
>> + case SEXT_EXPR:
>> + {
>> + gcc_assert (is_gimple_min_invariant (op1));
>> + unsigned int prec = tree_to_uhwi (op1);
>>
>> no need to assert, tree_to_uhwi will do that for you.
>>
>> + HOST_WIDE_INT may_be_nonzero = may_be_nonzero0.to_uhwi ();
>> + HOST_WIDE_INT must_be_nonzero = must_be_nonzero0.to_uhwi ();
>>
>> likewise with HOST_WIDE__INT issue.
>>
>> Otherwise looks ok to me. Btw, this and adding of SEXT_EXPR could be
>> accompanied with a match.pd pattern detecting sign-extension patterns,
>> that would give some extra test coverage.
>>
>> Thanks,
>> Richard.
>>
>>>
>>>
>>> gcc/ChangeLog:
>>>
>>> 2015-09-07 Kugan Vivekanandarajah <kug...@linaro.org>
>>>
>>> * tree-vrp.c (extract_range_from_binary_expr_1): Handle
>>> SEXT_EXPR.
>>> (simplify_bit_ops_using_ranges): Likewise.
>>> (simplify_stmt_using_ranges): Likewise.
