wide-int, rtl
Kenneth Zadeck
zadeck@naturalbridge.com
Fri Dec 6 21:06:00 GMT 2013
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.
>
done
> --- 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.
done
> --- 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?
the code no longer has restrictions on the size/mode of the variables.
> --- 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.
done
>
> {
> - 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?
done
>
> @@ -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?
it is what emacs did. I have formatted it back.
> @@ -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.
fixed
> + /* 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?
>
There is not really a faster track here. you still are starting with
a tree and converting to an rtx. All that the default one would do
would be to access the types precision and sign and use that.
> @@ -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?
fixed. this was just a mistake
>
> --- 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.
>
This requires some discussion as to the direction we want to go. This is
put in so that in gen_modes we can compute MAX_BITSIZE_MODE_ANY_INT and
MAX_BITSIZE_MODE_ANY_MODE. The problem is that during genmodes we do
have access to BITS_PER_UNIT. These two computed symbols are then
used as compile time constants in other parts of the compiler to
allocate data structures that are guaranteed to be large enough.
Richard Sandiford put this in so we would preserve the ability to build
a multi-targetted compiler where the targets had different values for
BITS_PER_UNIT. So one possibility is that we add some documentation to
this effect.
A different solution is to notice that there is already code in the
compiler that would not support BITS_PER_UNIT being anything but a
compile time constant, and so then change the genmodes code so that it
produced different symbols that were in terms of units rather than bits
and then convert the places in the compiler that use these new symbols
by multiplying by BITS_PER_UNIT there. However, that is really only
going to work if BITS_PER_UNIT is a compile time constant.
do you like one of these or do you have another suggestion?
> --- 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.
>
done
> --- 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?
yes. if unless the target has been converted to expect that integers
that do not fit into a single hwi are put in CONST_WIDE_INT, they remain
in double_ints.
> 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.
>
The data structure to hold a CONST_WIDE_INT is actually different than
the data structure that holds the rest of the rtx types. a
CONST_WIDE_INTs has a single variable length array in it. The length
of that array is determined by the size of the constant that it needs to
hold. At the suggestion of richi, we made the optimization that their
should only be a single one of these arrays allowed. This saved
several pointer indirections that would have been required had we made
it general enough to hop from variable sized element to variable sized
element as you do with the normal rtl scanning.
> --- 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?
>
we use the c++ std::make_pair to convert rtls to wide-ints. if we
ever add modes to rtl int constants, this would all go away. But there
was no reason to reinvent this given that it is part of every c++
implementation.
> @@ -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?
the comment now describes the reason behind this properly.
>
> --- 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.
I think that this may be more about me being honest than anything
else. In preparation for this patch we converted a lot of suspect
GEN_INTs to gen_int_mode, but there are a lot left. (these were pushed
directly to the trunk) The truth is that until we bite the bullet and
put modes in rtl integer constants this is always going to be a source
of bugs. The good news is that as targets move to supporting wide
ints, they will move away from one of the problematic parts of this
where the canonical way to test if a const_double has an int inside is
to test if the mode is VOIDmode.
>
> +#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?
I answered this earlier. but to reiterate, if you need big integers,
convert your port to TARGET_SUPPORTS_WIDE_INT, you cannot put a 180 bit
constant into 128 bits of data.
>
> + /* 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));
>
>
Eric,
Since there are a couple of open issues, i am not going ask for ok.
But the patch enclosed does do everything that is settled.
Kenny
-------------- next part --------------
A non-text attachment was scrubbed...
Name: RTL1.diff
Type: text/x-patch
Size: 5199 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20131206/726f7f66/attachment.bin>
More information about the Gcc-patches
mailing list