This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: wide-int, rtl


Eric,

Let me make one high level comment here and the low level comments will be responded to when i fix the patch.

CONST_DOUBLE has two hwis in it. So in practice, you get 128 bits and that is it. a CONST_WIDE_INT has an array of HWIs that has as many elements as it needs to represent the value.

If TARGET_SUPPORTS_WIDE_INT == 0 (the default and currently the value for every public port except the RS6000), then rtl integer constants that do not fit in a CONST_INT are put in a CONST_DOUBLE and there are never any CONST_WIDE_INTS created. A platform like this cannot correctly represent variables larger than 128 bits. i.e. this is like the current trunk except that there are fewer bugs for the 128 bit variables.

if TARGET_SUPPORTS_WIDE_INT is not 0, then rtl integer constants are put in CONST_WIDE_INT and CONST_DOUBLES only hold floating point values. For the last comment that you made, if the target does not support wide int, then there is simply no way to represent that constant properly and if the target accepts types larger than TImode, then the maintainer needs to do the work to use CONST_WIDE_INTs.

This is not a lot a work, but it requires target specific knowledge to convert the patterns, predicates and constraints to look for CONST_WIDE_INTs rather than CONST_DOUBLE for larger integers.

As far as your comment about const_double_operand, i could add some conditional code to make this not accept integers if the TARGET... is not 0. Richard Sandiford wanted my to use that test as infrequently as possible.

thanks for your comments.

Kenny

On 11/27/2013 11:24 AM, Eric Botcazou wrote:
Richi has asked the we break the wide-int patch so that the individual port
and front end maintainers can review their parts without have to go through
the entire patch.    This patch covers the first half of the rtl code.
--- a/gcc/cse.c
+++ b/gcc/cse.c
@@ -2336,15 +2336,23 @@ hash_rtx_cb (const_rtx x, enum machine_mode mode,
                 + (unsigned int) INTVAL (x));
        return hash;
+ case CONST_WIDE_INT:
+      {
+	int i;
+	for (i = 0; i < CONST_WIDE_INT_NUNITS (x); i++)
+	  hash += CONST_WIDE_INT_ELT (x, i);
+      }
+      return hash;

You can write "for (int i = 0; ..." now and remove the parentheses.


--- a/gcc/cselib.c
+++ b/gcc/cselib.c
@@ -1121,15 +1120,23 @@ cselib_hash_rtx (rtx x, int create, enum machine_mode
memmode)
        hash += ((unsigned) CONST_INT << 7) + INTVAL (x);
        return hash ? hash : (unsigned int) CONST_INT;
+ case CONST_WIDE_INT:
+      {
+	int i;
+	for (i = 0; i < CONST_WIDE_INT_NUNITS (x); i++)
+	  hash += CONST_WIDE_INT_ELT (x, i);
+      }
+      return hash;
+

Likewise.


--- a/gcc/expmed.c
+++ b/gcc/expmed.c
@@ -3298,23 +3268,13 @@ choose_multiplier (unsigned HOST_WIDE_INT d, int n,
int precision,
    pow = n + lgup;
    pow2 = n + lgup - precision;
- /* We could handle this with some effort, but this case is much
-     better handled directly with a scc insn, so rely on caller using
-     that.  */
-  gcc_assert (pow != HOST_BITS_PER_DOUBLE_INT);

Why removing it?



--- a/gcc/expr.c
+++ b/gcc/expr.c
@@ -722,64 +722,33 @@ convert_modes (enum machine_mode mode, enum machine_mode
oldmode, rtx x, int uns
    if (mode == oldmode)
      return x;
- /* There is one case that we must handle specially: If we are converting
-     a CONST_INT into a mode whose size is twice HOST_BITS_PER_WIDE_INT and
-     we are to interpret the constant as unsigned, gen_lowpart will do
-     the wrong if the constant appears negative.  What we want to do is
-     make the high-order word of the constant zero, not all ones.  */
-
-  if (unsignedp && GET_MODE_CLASS (mode) == MODE_INT
-      && GET_MODE_BITSIZE (mode) == HOST_BITS_PER_DOUBLE_INT
-      && CONST_INT_P (x) && INTVAL (x) < 0)
+  if (CONST_SCALAR_INT_P (x)
+      && GET_MODE_CLASS (mode) == MODE_INT)

On a single line.


      {
-      double_int val = double_int::from_uhwi (INTVAL (x));
-
-      /* We need to zero extend VAL.  */
-      if (oldmode != VOIDmode)
-	val = val.zext (GET_MODE_BITSIZE (oldmode));
-
-      return immed_double_int_const (val, mode);
+      /* If the caller did not tell us the old mode, then there is
+	 not much to do with respect to canonization.  We have to assume
+	 that all the bits are significant.  */
+      if (GET_MODE_CLASS (oldmode) != MODE_INT)
+	oldmode = MAX_MODE_INT;
+      wide_int w = wide_int::from (std::make_pair (x, oldmode),
+				   GET_MODE_PRECISION (mode),
+				   unsignedp ? UNSIGNED : SIGNED);
+      return immed_wide_int_const (w, mode);

canonicalization?


@@ -5301,10 +5271,10 @@ store_expr (tree exp, rtx target, int call_param_p,
bool nontemporal)
  			       &alt_rtl);
      }
- /* If TEMP is a VOIDmode constant and the mode of the type of EXP is not
-     the same as that of TARGET, adjust the constant.  This is needed, for
-     example, in case it is a CONST_DOUBLE and we want only a word-sized
-     value.  */
+  /* If TEMP is a VOIDmode constant and the mode of the type of EXP is
+     not the same as that of TARGET, adjust the constant.  This is
+     needed, for example, in case it is a CONST_DOUBLE or
+     CONST_WIDE_INT and we want only a word-sized value.  */
    if (CONSTANT_P (temp) && GET_MODE (temp) == VOIDmode
        && TREE_CODE (exp) != ERROR_MARK
        && GET_MODE (target) != TYPE_MODE (TREE_TYPE (exp)))

Why reformatting the whole comment?


@@ -9481,11 +9459,19 @@ expand_expr_real_1 (tree exp, rtx target, enum
machine_mode tmode,
        return decl_rtl;
case INTEGER_CST:
-      temp = immed_double_const (TREE_INT_CST_LOW (exp),
-				 TREE_INT_CST_HIGH (exp), mode);
-
-      return temp;
-
+      {
+	tree type = TREE_TYPE (exp);

Redundant, see the beginning of the function.

+	/* One could argue that GET_MODE_PRECISION (TYPE_MODE (type))
+	   should always be the same as TYPE_PRECISION (type).
+	   However, it is not.  Since we are converting from tree to
+	   rtl, we have to expose this ugly truth here.  */
+	temp = immed_wide_int_const (wide_int::from
+				       (exp,
+					GET_MODE_PRECISION (TYPE_MODE (type)),
+					TYPE_SIGN (type)),
+				     TYPE_MODE (type));
+	return temp;
+      }

I don't really see how one could argue that, given that there are much fewer
modes than possible type precisions, so please rewrite the comment, e.g.:

"Given that TYPE_PRECISION (type) is not always equal to
GET_MODE_PRECISION (TYPE_MODE (type)), we need to extend from the former
to the latter according to the signedness of the type".

What about a fast track where the precisions are indeed equal?



@@ -11145,8 +11131,8 @@ const_vector_from_tree (tree exp)
  	RTVEC_ELT (v, i) = CONST_FIXED_FROM_FIXED_VALUE (TREE_FIXED_CST (elt),
  							 inner);
        else
-	RTVEC_ELT (v, i) = immed_double_int_const (tree_to_double_int (elt),
-						   inner);
+	RTVEC_ELT (v, i)
+	  = immed_wide_int_const (elt, TYPE_MODE (TREE_TYPE (elt)));
      }
return gen_rtx_CONST_VECTOR (mode, v);

Why replacing inner?


--- a/gcc/machmode.def
+++ b/gcc/machmode.def
@@ -229,6 +229,9 @@ UACCUM_MODE (USA, 4, 16, 16); /* 16.16 */
  UACCUM_MODE (UDA, 8, 32, 32); /* 32.32 */
  UACCUM_MODE (UTA, 16, 64, 64); /* 64.64 */
+/* Should be overridden by EXTRA_MODES_FILE if wrong. */
+#define MAX_BITS_PER_UNIT 8
+

What is it for?  It's not documented at all.


--- a/gcc/print-rtl.c
+++ b/gcc/print-rtl.c
@@ -617,6 +617,12 @@ print_rtx (const_rtx in_rtx)
  	  fprintf (outfile, " [%s]", s);
  	}
        break;
+
+    case CONST_WIDE_INT:
+      if (! flag_simple)
+	fprintf (outfile, " ");
+      cwi_output_hex (outfile, in_rtx);
+      break;
  #endif
Remove the if (! flag_simple) test.


--- a/gcc/recog.c
+++ b/gcc/recog.c

  /* Returns 1 if OP is an operand that is a constant integer or constant
-   floating-point number.  */
+   floating-point number of MODE.  */
+
+int
+const_double_operand (rtx op, enum machine_mode mode)
+{
+  return (GET_CODE (op) == CONST_DOUBLE)
+	  && (GET_MODE (op) == mode || mode == VOIDmode);
+}

Can CONST_DOUBLEs still represent constant integers?


diff --git a/gcc/rtl.def b/gcc/rtl.def
index 15a997b..a76b28b 100644
--- a/gcc/rtl.def
+++ b/gcc/rtl.def
@@ -345,6 +345,9 @@ DEF_RTL_EXPR(TRAP_IF, "trap_if", "ee", RTX_EXTRA)
  /* numeric integer constant */
  DEF_RTL_EXPR(CONST_INT, "const_int", "w", RTX_CONST_OBJ)
+/* numeric integer constant */
+DEF_RTL_EXPR(CONST_WIDE_INT, "const_wide_int", "", RTX_CONST_OBJ)
+

I think that we should consider using a new letter here, for example W.
"" is reserved to very specific cases.


--- a/gcc/rtl.h
+++ b/gcc/rtl.h
@@ -20,6 +20,7 @@ along with GCC; see the file COPYING3.  If not see
  #ifndef GCC_RTL_H
  #define GCC_RTL_H
+#include <utility>

Ouch.  Can't this really be avoided?


@@ -336,6 +348,14 @@ struct GTY((chain_next ("RTX_NEXT (&%h)"),
       1 in a VALUE or DEBUG_EXPR is NO_LOC_P in var-tracking.c.  */
    unsigned return_val : 1;
+ union {
+    /* RTXs are free to use up to 32 bit from here.  */
+
+    /* In a CONST_WIDE_INT (aka hwivec_def), this is the number of
+       HOST_WIDE_INTs in the hwivec_def.  */
+    unsigned GTY ((tag ("CONST_WIDE_INT"))) num_elem:32;
+  } GTY ((desc ("GET_CODE (&%0)"))) u2;
+
    /* The first element of the operands of this rtx.
       The number of operands and their types are controlled
       by the `code' field, according to rtl.def.  */

Why can't this be put into hwivec_def?


--- a/gcc/simplify-rtx.c
+++ b/gcc/simplify-rtx.c

+      if (op_mode == VOIDmode)
+	{
+	  /* CONST_INT have VOIDmode as the mode.  We assume that all
+	     the bits of the constant are significant, though, this is
+	     a dangerous assumption as many times CONST_INTs are
+	     created and used with garbage in the bits outside of the
+	     precision of the implied mode of the const_int.  */
+	  op_mode = MAX_MODE_INT;
+	}

Well, if that's really dangerous, we should find another way or at least
stop the compiler before generating wrong code.


+#if TARGET_SUPPORTS_WIDE_INT == 0
+      /* This assert keeps the simplification from producing a result
+	 that cannot be represented in a CONST_DOUBLE but a lot of
+	 upstream callers expect that this function never fails to
+	 simplify something and so you if you added this to the test
+	 above the code would die later anyway.  If this assert
+	 happens, you just need to make the port support wide int.  */
+      gcc_assert (width <= HOST_BITS_PER_DOUBLE_INT);
+#endif

Can't we be more forgiving here and in the other similar places?


+  /* We support any size mode.  */
+  max_bitsize = MAX (GET_MODE_BITSIZE (outermode),
+		     GET_MODE_BITSIZE (innermode));

max_bitsize
   = MAX (GET_MODE_BITSIZE (outermode), GET_MODE_BITSIZE (innermode));




Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]