diff mbox

[1/7] Add new tree code SEXT_EXPR

Message ID 561A3B7E.2060405@linaro.org
State New
Headers show

Commit Message

Kugan Vivekanandarajah Oct. 11, 2015, 10:35 a.m. UTC 
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?


Thanks,
Kugan

Comments

Richard Biener Oct. 12, 2015, 12:21 p.m. UTC | #1 
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
diff mbox

Patch

From 2326daf0e7088e01e87574c9824b1c7248395798 Mon Sep 17 00:00:00 2001
From: Kugan Vivekanandarajah <kugan.vivekanandarajah@linaro.org>
Date: Mon, 17 Aug 2015 13:37:15 +1000
Subject: [PATCH 1/7] Add new SEXT_EXPR tree code

---
 gcc/cfgexpand.c         | 10 ++++++++++
 gcc/expr.c              | 20 ++++++++++++++++++++
 gcc/fold-const.c        |  4 ++++
 gcc/tree-cfg.c          | 13 +++++++++++++
 gcc/tree-inline.c       |  1 +
 gcc/tree-pretty-print.c | 11 +++++++++++
 gcc/tree.def            |  5 +++++
 7 files changed, 64 insertions(+)

diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index 58e55d2..dea0e37 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -5057,6 +5057,16 @@  expand_debug_expr (tree exp)
     case FMA_EXPR:
       return simplify_gen_ternary (FMA, mode, inner_mode, op0, op1, op2);
 
+    case SEXT_EXPR:
+      gcc_assert (CONST_INT_P (op1));
+      inner_mode = mode_for_size (INTVAL (op1), MODE_INT, 0);
+      if (mode != inner_mode)
+	op0 = simplify_gen_unary (SIGN_EXTEND,
+				  mode,
+				  gen_lowpart_SUBREG (inner_mode, op0),
+				  inner_mode);
+      return op0;
+
     default:
     flag_unsupported:
 #ifdef ENABLE_CHECKING
diff --git a/gcc/expr.c b/gcc/expr.c
index 0bbfccd..30898a2 100644
--- a/gcc/expr.c
+++ b/gcc/expr.c
@@ -9296,6 +9296,26 @@  expand_expr_real_2 (sepops ops, rtx target, machine_mode tmode,
       target = expand_vec_cond_expr (type, treeop0, treeop1, treeop2, target);
       return target;
 
+    case SEXT_EXPR:
+	{
+	  machine_mode inner_mode = mode_for_size (tree_to_shwi (treeop1),
+						   MODE_INT, 0);
+	  rtx temp, result;
+	  rtx op0 = expand_normal (treeop0);
+	  op0 = force_reg (mode, op0);
+	  if (mode != inner_mode)
+	    {
+	      result = gen_reg_rtx (mode);
+	      temp = simplify_gen_unary (SIGN_EXTEND, mode,
+					 gen_lowpart_SUBREG (inner_mode, op0),
+					 inner_mode);
+	      convert_move (result, temp, 0);
+	    }
+	  else
+	    result = op0;
+	  return result;
+	}
+
     default:
       gcc_unreachable ();
     }
diff --git a/gcc/fold-const.c b/gcc/fold-const.c
index 7231fd6..d693b42 100644
--- a/gcc/fold-const.c
+++ b/gcc/fold-const.c
@@ -984,6 +984,10 @@  int_const_binop_1 (enum tree_code code, const_tree arg1, const_tree parg2,
       res = wi::bit_and (arg1, arg2);
       break;
 
+    case SEXT_EXPR:
+      res = wi::sext (arg1, arg2.to_uhwi ());
+      break;
+
     case RSHIFT_EXPR:
     case LSHIFT_EXPR:
       if (wi::neg_p (arg2))
diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index 712d8cc..97da2f3 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -3756,6 +3756,19 @@  verify_gimple_assign_binary (gassign *stmt)
         return false;
       }
 
+    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)
+	  {
+	    error ("invalid operands in sext expr");
+	    return true;
+	  }
+	return false;
+      }
+
     case VEC_WIDEN_LSHIFT_HI_EXPR:
     case VEC_WIDEN_LSHIFT_LO_EXPR:
       {
diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c
index ac9586e..0975730 100644
--- a/gcc/tree-inline.c
+++ b/gcc/tree-inline.c
@@ -3884,6 +3884,7 @@  estimate_operator_cost (enum tree_code code, eni_weights *weights,
     case BIT_XOR_EXPR:
     case BIT_AND_EXPR:
     case BIT_NOT_EXPR:
+    case SEXT_EXPR:
 
     case TRUTH_ANDIF_EXPR:
     case TRUTH_ORIF_EXPR:
diff --git a/gcc/tree-pretty-print.c b/gcc/tree-pretty-print.c
index 7cd1fe7..efd8d5b 100644
--- a/gcc/tree-pretty-print.c
+++ b/gcc/tree-pretty-print.c
@@ -1794,6 +1794,14 @@  dump_generic_node (pretty_printer *pp, tree node, int spc, int flags,
       }
       break;
 
+    case SEXT_EXPR:
+      pp_string (pp, "SEXT_EXPR <");
+      dump_generic_node (pp, TREE_OPERAND (node, 0), spc, flags, false);
+      pp_string (pp, ", ");
+      dump_generic_node (pp, TREE_OPERAND (node, 1), spc, flags, false);
+      pp_greater (pp);
+      break;
+
     case MODIFY_EXPR:
     case INIT_EXPR:
       dump_generic_node (pp, TREE_OPERAND (node, 0), spc, flags,
@@ -3414,6 +3422,9 @@  op_symbol_code (enum tree_code code)
     case MIN_EXPR:
       return "min";
 
+    case SEXT_EXPR:
+      return "sext";
+
     default:
       return "<<< ??? >>>";
     }
diff --git a/gcc/tree.def b/gcc/tree.def
index 56580af..48e7413 100644
--- a/gcc/tree.def
+++ b/gcc/tree.def
@@ -752,6 +752,11 @@  DEFTREECODE (BIT_XOR_EXPR, "bit_xor_expr", tcc_binary, 2)
 DEFTREECODE (BIT_AND_EXPR, "bit_and_expr", tcc_binary, 2)
 DEFTREECODE (BIT_NOT_EXPR, "bit_not_expr", tcc_unary, 1)
 
+/*  Sign-extend operation.  It will sign extend first operand from
+ the sign bit specified by the second operand.  The type of the
+ result is that of the first operand.  */
+DEFTREECODE (SEXT_EXPR, "sext_expr", tcc_binary, 2)
+
 /* ANDIF and ORIF allow the second operand not to be computed if the
    value of the expression is determined from the first operand.  AND,
    OR, and XOR always compute the second operand whether its value is
-- 
1.9.1