[1/7] Add new tree code SEXT_EXPR

[Richard Biener]

On Sun, Oct 11, 2015 at 12:35 PM, Kugan
<kugan.vivekanandarajah@linaro.org> wrote:
>
>
> On 15/09/15 23:18, Richard Biener wrote:
>> On Mon, Sep 7, 2015 at 4:55 AM, Kugan <kugan.vivekanandarajah@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