On Fri, Oct 5, 2012 at 2:39 PM, Richard Guenther
<richard.guent...@gmail.com> wrote:
>
> Ok, I see where you are going. Let me look at the patch again.
* The introduction and use of CONST_SCALAR_INT_P could be split out
(obvious and good)
* DEF_RTL_EXPR(CONST_WIDE_INT, "const_wide_int", "", RTX_CONST_OBJ)
defining that new RTX is orthogonal to providing the wide_int abstraction
for operating on CONST_INT and CONST_DOUBLE, right?
@@ -144,6 +144,7 @@ static bool
prefer_and_bit_test (enum machine_mode mode, int bitnum)
{
bool speed_p;
+ wide_int mask = wide_int::set_bit_in_zero (bitnum, mode);
set_bit_in_zero ... why use this instead of
wide_int::zero (mode).set_bit (bitnum) that would match the old
double_int_zero.set_bit interface and be more composed of primitives?
if (and_test == 0)
{
@@ -164,8 +165,7 @@ prefer_and_bit_test (enum machine_mode m
}
/* Fill in the integers. */
- XEXP (and_test, 1)
- = immed_double_int_const (double_int_zero.set_bit (bitnum), mode);
+ XEXP (and_test, 1) = immed_wide_int_const (mask);
I suppose immed_wide_int_const may create a CONST_INT, a CONST_DOUBLE
or a CONST_WIDE_INT?
+#if TARGET_SUPPORTS_WIDE_INT
+/* Returns 1 if OP is an operand that is a CONST_INT or CONST_WIDE_INT. */
+int
+const_scalar_int_operand (rtx op, enum machine_mode mode)
+{
why is it necessary to condition anything on TARGET_SUPPORTS_WIDE_INT?
It seems testing/compile coverage of this code will be bad ...
Can't you simply map CONST_WIDE_INT to CONST_DOUBLE for targets
not supporting CONST_WIDE_INT (what does it mean to "support"
CONST_WIDE_INT? Does a target that supports CONST_WIDE_INT no
longer use CONST_DOUBLEs for integers?)
+ if (!CONST_WIDE_INT_P (op))
+ return 0;
hm, that doesn't match the comment (CONST_INT is supposed to return 1 as well).
The comment doesn't really tell what the function does it seems,
+ int prec = GET_MODE_PRECISION (mode);
+ int bitsize = GET_MODE_BITSIZE (mode);
+
+ if (CONST_WIDE_INT_NUNITS (op) * HOST_BITS_PER_WIDE_INT > bitsize)
+ return 0;
it's mode argument is not documented. And this doesn't even try to see if
the constant would fit the mode anyway (like if it's zero). Not sure what
the function is for.
+ {
+ /* Multiword partial int. */
+ HOST_WIDE_INT x
+ = CONST_WIDE_INT_ELT (op, CONST_WIDE_INT_NUNITS (op) - 1);
+ return (wide_int::sext (x, prec & (HOST_BITS_PER_WIDE_INT - 1))
+ == x);
err - so now we have wide_int with a mode but we pass in another mode
and see if they have the same value? Same value as-in signed value?
Which probably is why you need to rule out different size constants above?
Because you don't know whether to sign or zero extend?
+/* Returns 1 if OP is an operand that is a CONST_WIDE_INT. */
+int
+const_wide_int_operand (rtx op, enum machine_mode mode)
+{
similar comments, common code should be factored out at least.
Instead of conditioning a whole set of new function on supports-wide-int
please add cases to the existing functions to avoid diverging in pieces
that both kind of targets support.
@@ -2599,7 +2678,7 @@ constrain_operands (int strict)
break;
case 'n':
- if (CONST_INT_P (op) || CONST_DOUBLE_AS_INT_P (op))
+ if (CONST_SCALAR_INT_P (op))
win = 1;
factoring out this changes would really make the patch less noisy.
skipping to rtl.h bits
+struct GTY((variable_size)) hwivec_def {
+ int num_elem; /* number of elements */
+ HOST_WIDE_INT elem[1];
+};
num_elem seems redundant and computable from GET_MODE_SIZE.
Or do you want to optimize encoding like for CONST_INT (and unlike
CONST_DOUBLE)? I doubt the above packs nicely into rtx_def?
A general issue of it though - we waste 32bits on 64bit hosts in
rtx_def between the bits and the union. Perfect place for num_elem
(if you really need it) and avoids another 4 byte hole. Similar issue
exists in rtvec_def.
back to where I was
@@ -645,6 +673,10 @@ iterative_hash_rtx (const_rtx x, hashval
return iterative_hash_object (i, hash);
case CONST_INT:
return iterative_hash_object (INTVAL (x), hash);
+ case CONST_WIDE_INT:
+ for (i = 0; i < CONST_WIDE_INT_NUNITS (x); i++)
+ hash = iterative_hash_object (CONST_WIDE_INT_ELT (x, i), hash);
+ return hash;
I see CONST_DOUBLEs value is not hashed. Why hash wide-ints value?
Seeing
+#define HWI_GET_NUM_ELEM(HWIVEC) ((HWIVEC)->num_elem)
+#define HWI_PUT_NUM_ELEM(HWIVEC, NUM) ((HWIVEC)->num_elem = (NUM))
skipping to
+#if TARGET_SUPPORTS_WIDE_INT
+ {
+ rtx value = const_wide_int_alloc (len);
+ unsigned int i;
+
+ /* It is so tempting to just put the mode in here. Must control
+ myself ... */
+ PUT_MODE (value, VOIDmode);
+ HWI_PUT_NUM_ELEM (CONST_WIDE_INT_VEC (value), len);
why is NUM_ELEM not set in const_wide_int_alloc, and only there?
+extern rtx rtx_alloc_stat_v (RTX_CODE MEM_STAT_DECL, int);
+#define rtx_alloc_v(c, SZ) rtx_alloc_stat_v (c MEM_STAT_INFO, SZ)
+#define const_wide_int_alloc(NWORDS) \
+ rtx_alloc_v (CONST_WIDE_INT, \
+ (sizeof (struct hwivec_def) \
+ + ((NWORDS)-1) * sizeof (HOST_WIDE_INT))) \
because it's a macro(?). Ugh.
I see CONST_DOUBLE for ints and CONST_WIDE_INT for ints use
is mutually exclusive. Good. What's the issue with converting targets?
Just retain the existing 2 * HWI limit by default.
+#if TARGET_SUPPORTS_WIDE_INT
+
+/* Match CONST_*s that can represent compile-time constant integers. */
+#define CASE_CONST_SCALAR_INT \
+ case CONST_INT: \
+ case CONST_WIDE_INT
+
I regard this abstraction is mostly because the transition is not finished.
Not sure if seeing CASE_CONST_SCALAR_INT, CASE_CONST_UNIQUE (?),
CASE_CONST_ANY adds more to the confusion than spelling out all of them.
Isn't there sth like a tree-code-class for RTXen? That would catch
CASE_CONST_ANY, no?
@@ -3081,6 +3081,8 @@ commutative_operand_precedence (rtx op)
/* Constants always come the second operand. Prefer "nice" constants. */
if (code == CONST_INT)
return -8;
+ if (code == CONST_WIDE_INT)
+ return -8;
if (code == CONST_DOUBLE)
return -7;
if (code == CONST_FIXED)
I think it should be the same as CONST_DOUBLE which it "replaces".
Likewise below, that avoids code generation changes (which there should
be none for a target that is converted, right?).
@@ -5351,6 +5355,20 @@ split_double (rtx value, rtx *first, rtx
}
}
}
+ else if (GET_CODE (value) == CONST_WIDE_INT)
+ {
+ gcc_assert (CONST_WIDE_INT_NUNITS (value) == 2);
+ if (WORDS_BIG_ENDIAN)
+ {
+ *first = GEN_INT (CONST_WIDE_INT_ELT (value, 1));
+ *second = GEN_INT (CONST_WIDE_INT_ELT (value, 0));
+ }
+ else
+ {
+ *first = GEN_INT (CONST_WIDE_INT_ELT (value, 0));
+ *second = GEN_INT (CONST_WIDE_INT_ELT (value, 1));
+ }
+ }
scary ;)
Index: gcc/sched-vis.c
===================================================================
--- gcc/sched-vis.c (revision 191978)
+++ gcc/sched-vis.c (working copy)
@@ -444,14 +444,31 @@ print_value (char *buf, const_rtx x, int
...
case CONST_DOUBLE:
- if (FLOAT_MODE_P (GET_MODE (x)))
- real_to_decimal (t, CONST_DOUBLE_REAL_VALUE (x), sizeof (t), 0, 1);
- else
+ if (TARGET_SUPPORTS_WIDE_INT == 0 && !FLOAT_MODE_P (GET_MODE (x)))
sprintf (t,
"<" HOST_WIDE_INT_PRINT_HEX "," HOST_WIDE_INT_PRINT_HEX ">",
(unsigned HOST_WIDE_INT) CONST_DOUBLE_LOW (x),
(unsigned HOST_WIDE_INT) CONST_DOUBLE_HIGH (x));
+ else
+ real_to_decimal (t, CONST_DOUBLE_REAL_VALUE (x), sizeof (t), 0, 1);
cur = safe_concat (buf, cur, t);
break;
I don't see that this hunk is needed? In fact it's more confusing this way.
+/* If the target supports integers that are wider than two
+ HOST_WIDE_INTs on the host compiler, then the target should define
+ TARGET_SUPPORTS_WIDE_INT and make the appropriate fixups.
+ Otherwise the compiler really is not robust. */
+#ifndef TARGET_SUPPORTS_WIDE_INT
+#define TARGET_SUPPORTS_WIDE_INT 0
+#endif
I wonder what are the appropriate fixups?
-/* Return a CONST_INT or CONST_DOUBLE corresponding to target reading
- GET_MODE_BITSIZE (MODE) bits from string constant STR. */
+/* Return a CONST_INT, CONST_WIDE_INT, or CONST_DOUBLE corresponding
+ to target reading GET_MODE_BITSIZE (MODE) bits from string constant
+ STR. */
maybe being less specific in this kind of comments and just refering to
RTX integer constants would be better?
... skip ...
Index: gcc/hwint.c
===================================================================
--- gcc/hwint.c (revision 191978)
+++ gcc/hwint.c (working copy)
@@ -112,6 +112,29 @@ ffs_hwi (unsigned HOST_WIDE_INT x)
int
popcount_hwi (unsigned HOST_WIDE_INT x)
{
+ /* Compute the popcount of a HWI using the algorithm from
+ Hacker's Delight. */
+#if HOST_BITS_PER_WIDE_INT == 32
separate patch please. With a rationale.
...
+/* Constructs tree in type TYPE from with value given by CST. Signedness
+ of CST is assumed to be the same as the signedness of TYPE. */
+
+tree
+wide_int_to_tree (tree type, const wide_int &cst)
+{
+ wide_int v;
+ if (TYPE_UNSIGNED (type))
+ v = cst.zext (TYPE_PRECISION (type));
+ else
+ v = cst.sext (TYPE_PRECISION (type));
+
+ return build_int_cst_wide (type, v.elt (0), v.elt (1));
this needs an assert that the wide-int contains two HWIs.
+#ifndef GENERATOR_FILE
+extern tree wide_int_to_tree (tree type, const wide_int &cst);
+#endif
why's that? The header inclusion isn't guarded that way.
Stopping here, skipping to wide-int.[ch]
+#define DEBUG_WIDE_INT
+#ifdef DEBUG_WIDE_INT
+ /* Debugging routines. */
+ static void debug_vw (const char* name, int r, const wide_int& o0);
+ static void debug_vwh (const char* name, int r, const wide_int &o0,
+ HOST_WIDE_INT o1);
there is DEBUG_FUNCTION.
+/* This is the maximal size of the buffer needed for dump. */
+const int MAX = (MAX_BITSIZE_MODE_ANY_INT / 4
+ + MAX_BITSIZE_MODE_ANY_INT / HOST_BITS_PER_WIDE_INT + 32);
I'd prefer a target macro for this, not anything based off
MAX_BITSIZE_MODE_ANY_INT just to allow no-op transition of a
target to wide-ints (which IMHO should be the default for all targets,
simplifying the patch).
+wide_int
+wide_int::from_uhwi (unsigned HOST_WIDE_INT op0, enum machine_mode
mode, bool *overflow)
+{
...
+#ifdef DEBUG_WIDE_INT
+ if (dump_file)
+ {
+ char buf0[MAX];
+ fprintf (dump_file, "%s: %s = " HOST_WIDE_INT_PRINT_HEX "\n",
+ "wide_int::from_uhwi", result.dump (buf0), op0);
+ }
+#endif
hm, ok ... I suppose the debug stuff (which looks very noisy) is going to be
removed before this gets committed anywhere?
+wide_int
+wide_int::from_int_cst (const_tree tcst)
+{
+#if 1
+ wide_int result;
+ tree type = TREE_TYPE (tcst);
should just call wide_it::from_double_int (tree_to_double_int (tcst))
As I said elsewhere I don't think only allowing precisions that are somewhere
in any mode is good enough. We need the actual precision here (everywhere).
Index: gcc/wide-int.h
===================================================================
--- gcc/wide-int.h (revision 0)
+++ gcc/wide-int.h (revision 0)
@@ -0,0 +1,718 @@
+/* Operations with very long integers.
+ Copyright (C) 2012 Free Software Foundation, Inc.
...
+
+#ifndef GENERATOR_FILE
+#include "hwint.h"
+#include "options.h"
+#include "tm.h"
+#include "insn-modes.h"
+#include "machmode.h"
+#include "double-int.h"
+#include <gmp.h>
+#include "insn-modes.h"
ugh. Compare this with double-int.h which just needs <gmp.h> (and even that
is a red herring) ...
+#define wide_int_minus_one(MODE) (wide_int::from_shwi (-1, MODE))
+#define wide_int_zero(MODE) (wide_int::from_shwi (0, MODE))
+#define wide_int_one(MODE) (wide_int::from_shwi (1, MODE))
+#define wide_int_two(MODE) (wide_int::from_shwi (2, MODE))
+#define wide_int_ten(MODE) (wide_int::from_shwi (10, MODE))
add static functions like
wide_int wide_int::zero (MODE)
+class wide_int {
+ /* Internal representation. */
+ HOST_WIDE_INT val[MAX_BITSIZE_MODE_ANY_INT / HOST_BITS_PER_WIDE_INT];
+ unsigned short len;
+ enum machine_mode mode;
you really need to store the precision here, not the mode. We do not have
a distinct mode for each of the tree constant precisions we see. So what
might work for RTL will later fail on trees, so better do it correct
from the start
(overloads using mode rather than precision may be acceptable - similarly
I'd consider overloads for tree types).
len seems redundant unless you want to optimize encoding.
len == (precision + HOST_BITS_PER_WIDE_INT - 1) / HOST_BITS_PER_WIDE_INT.
+ enum Op {
+ NONE,
we don't have sth like this in double-int.h, why was the double-int mechanism
not viable?
+ static wide_int from_int_cst (const_tree);
+ static wide_int from_rtx (const_rtx, enum machine_mode);
from_tree instead of from_int_cst? I miss from_pair.
+ HOST_WIDE_INT to_shwi (int prec) const;
isn't the precision encoded? If it's a different precision this seems to
combine two primitives - is that really that convenient (if so that hints
at some oddity IMHO).
+ static wide_int max_value (const enum machine_mode mode, Op sgn);
+ static wide_int max_value (const enum machine_mode mode, int prec, Op sgn);
+ static wide_int min_value (const enum machine_mode mode, Op sgn);
+ static wide_int min_value (const enum machine_mode mode, int prec, Op sgn);
again prec seems redundant to me. double-int uses 'bool uns', why is this
different here? What happens for NONE, TRUNC?
+ inline unsigned short get_len () const;
+ inline void set_len (unsigned int);
the length should be an implementation detail, no? Important is only
the precision of the number. So this shouldn' t be exported at least.
+ inline int full_len () const;
what's this?
+ inline enum machine_mode get_mode () const;
+ inline void set_mode (enum machine_mode m);
not sure if we want machine-mode/precision mutating operations (similar
to length mutating operations). I guess better not.
+ wide_int copy (enum machine_mode mode) const;
why does this need a mode argument? Maybe 'copy' isn't a good name?
+ static inline HOST_WIDE_INT sext (HOST_WIDE_INT src, int prec);
+ static inline HOST_WIDE_INT zext (HOST_WIDE_INT src, int prec);
doesn't belong to wide_int::
+#define wide_int_smin(OP0,OP1) ((OP0).lts_p (OP1) ? (OP0) : (OP1))
+#define wide_int_smax(OP0,OP1) ((OP0).lts_p (OP1) ? (OP1) : (OP0))
+#define wide_int_umin(OP0,OP1) ((OP0).ltu_p (OP1) ? (OP0) : (OP1))
+#define wide_int_umax(OP0,OP1) ((OP0).ltu_p (OP1) ? (OP1) : (OP0))
use inline functions
+void
+wide_int::set_len (unsigned int l)
+{
+ gcc_assert (l < MAX_BITSIZE_MODE_ANY_INT / HOST_BITS_PER_WIDE_INT);
use gcc_checking_assert everywhere.
Overall it's not too bad. The mode vs. precision thing needs to be sorted
out and I'd like TARGET_SUPPORTS_WIDE_INT not be introduced,
even transitional. Just make it possible to have the default be the
existing size of CONST_DOUBLE.
Thanks,
Richard.
