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.
Otherwise ok for trunk.
Thanks,
Richard.
>
> Thanks,
> Kugan
