On Thu, Oct 15, 2015 at 7:49 AM, Kugan
<kugan.vivekanandara...@linaro.org> wrote:
>
>
> On 12/10/15 23:21, Richard Biener wrote:
>> On Sun, Oct 11, 2015 at 12:35 PM, Kugan
>> <kugan.vivekanandara...@linaro.org> wrote:
>>>
>>>
>>> On 15/09/15 23:18, Richard Biener wrote:
>>>> On Mon, Sep 7, 2015 at 4:55 AM, Kugan <kugan.vivekanandara...@linaro.org>
>>>> wrote:
>>>>>
>>>>> This patch adds support for new tree code SEXT_EXPR.
>>>>
>>>> diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
>>>> index d567a87..bbc3c10 100644
>>>> --- a/gcc/cfgexpand.c
>>>> +++ b/gcc/cfgexpand.c
>>>> @@ -5071,6 +5071,10 @@ expand_debug_expr (tree exp)
>>>> case FMA_EXPR:
>>>> return simplify_gen_ternary (FMA, mode, inner_mode, op0, op1, op2);
>>>>
>>>> + case SEXT_EXPR:
>>>> + return op0;
>>>>
>>>> that looks wrong. Generate (sext:... ) here?
>>>>
>>>> + case SEXT_EXPR:
>>>> + {
>>>> + rtx op0 = expand_normal (treeop0);
>>>> + rtx temp;
>>>> + if (!target)
>>>> + target = gen_reg_rtx (TYPE_MODE (TREE_TYPE (treeop0)));
>>>> +
>>>> + machine_mode inner_mode
>>>> + = smallest_mode_for_size (tree_to_shwi (treeop1),
>>>> + MODE_INT);
>>>> + temp = convert_modes (inner_mode,
>>>> + TYPE_MODE (TREE_TYPE (treeop0)), op0, 0);
>>>> + convert_move (target, temp, 0);
>>>> + return target;
>>>> + }
>>>>
>>>> Humm - is that really how we expand sign extensions right now? No helper
>>>> that would generate (sext ...) directly? I wouldn't try using 'target'
>>>> btw but
>>>> simply return (sext:mode op0 op1) or so. But I am no way an RTL expert.
>>>>
>>>> Note that if we don't disallow arbitrary precision SEXT_EXPRs we have to
>>>> fall back to using shifts (and smallest_mode_for_size is simply wrong).
>>>>
>>>> + case SEXT_EXPR:
>>>> + {
>>>> + if (!INTEGRAL_TYPE_P (lhs_type)
>>>> + || !INTEGRAL_TYPE_P (rhs1_type)
>>>> + || TREE_CODE (rhs2) != INTEGER_CST)
>>>>
>>>> please constrain this some more, with
>>>>
>>>> || !useless_type_conversion_p (lhs_type, rhs1_type)
>>>>
>>>> + {
>>>> + error ("invalid operands in sext expr");
>>>> + return true;
>>>> + }
>>>> + return false;
>>>> + }
>>>>
>>>> @@ -3414,6 +3422,9 @@ op_symbol_code (enum tree_code code)
>>>> case MIN_EXPR:
>>>> return "min";
>>>>
>>>> + case SEXT_EXPR:
>>>> + return "sext from bit";
>>>> +
>>>>
>>>> just "sext" please.
>>>>
>>>> +/* Sign-extend operation. It will sign extend first operand from
>>>> + the sign bit specified by the second operand. */
>>>> +DEFTREECODE (SEXT_EXPR, "sext_expr", tcc_binary, 2)
>>>>
>>>> "from the INTEGER_CST sign bit specified"
>>>>
>>>> Also add "The type of the result is that of the first operand."
>>>>
>>>
>>>
>>>
>>> Thanks for the review. Attached patch attempts to address the above
>>> comments. Does this look better?
>>
>> + case SEXT_EXPR:
>> + gcc_assert (CONST_INT_P (op1));
>> + inner_mode = mode_for_size (INTVAL (op1), MODE_INT, 0);
>>
>> We should add
>>
>> gcc_assert (GET_MODE_BITSIZE (inner_mode) == INTVAL (op1));
>>
>> + if (mode != inner_mode)
>> + op0 = simplify_gen_unary (SIGN_EXTEND,
>> + mode,
>> + gen_lowpart_SUBREG (inner_mode, op0),
>> + inner_mode);
>>
>> as we're otherwise silently dropping things like SEXT (short-typed-var, 13)
>>
>> + case SEXT_EXPR:
>> + {
>> + machine_mode inner_mode = mode_for_size (tree_to_shwi (treeop1),
>> + MODE_INT, 0);
>>
>> Likewise. Also treeop1 should be unsigned, thus tree_to_uhwi?
>>
>> + rtx temp, result;
>> + rtx op0 = expand_normal (treeop0);
>> + op0 = force_reg (mode, op0);
>> + if (mode != inner_mode)
>> + {
>>
>> Again, for the RTL bits I'm not sure they are correct. For example I don't
>> see why we need a lowpart SUBREG, isn't a "regular" SUBREG enough?
>>
>> + case SEXT_EXPR:
>> + {
>> + if (!INTEGRAL_TYPE_P (lhs_type)
>> + || !useless_type_conversion_p (lhs_type, rhs1_type)
>> + || !INTEGRAL_TYPE_P (rhs1_type)
>> + || TREE_CODE (rhs2) != INTEGER_CST)
>>
>> the INTEGRAL_TYPE_P (rhs1_type) check is redundant with
>> the useless_type_Conversion_p one. Please check
>> tree_fits_uhwi (rhs2) instead of != INTEGER_CST.
>>
>
> Thanks for the review, Please find the updated patch based on the review
> comments.
Ok.
Thanks,
Richard.
> Thanks,
> Kugan
