Message ID | 52A23C51.8040107@naturalbridge.com |
---|---|
State | New |
Headers | show |
Kenneth Zadeck <zadeck@naturalbridge.com> writes: >> + /* 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. FWIW it would be: temp = immed_wide_int_const (exp, TYPE_MODE (type)); But it's hard to tell whether it would buy much. It didn't show up as a hot spot when I was doing performance measurements before. >> --- 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. Sorry, I forgot yesterday an important detail behind why this seemed like a good thing. I think there was a strong feeling (from me and others) that wide-int.h shouldn't depend on tm.h. If we make wide-int.h depend on tm.h then basically all the compiler does. So as it stands we can't use BITS_PER_UNIT directly. Having a MAX_BITS_PER_UNIT for "all compiled-in targets" (which obviously as things stand is exactly one) seemed like a reasonable abstraction. Alternatively we could say that BITS_PER_UNIT is really part of the definition of QImode and move it to the modes.def file. Thanks, Richard
On Sat, Dec 7, 2013 at 11:28 AM, Richard Sandiford <rdsandiford@googlemail.com> wrote: > Kenneth Zadeck <zadeck@naturalbridge.com> writes: >>> + /* 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. > > FWIW it would be: > > temp = immed_wide_int_const (exp, TYPE_MODE (type)); > > But it's hard to tell whether it would buy much. It didn't show up as > a hot spot when I was doing performance measurements before. > >>> --- 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. > > Sorry, I forgot yesterday an important detail behind why this seemed > like a good thing. I think there was a strong feeling (from me and others) > that wide-int.h shouldn't depend on tm.h. If we make wide-int.h depend > on tm.h then basically all the compiler does. > > So as it stands we can't use BITS_PER_UNIT directly. Having a > MAX_BITS_PER_UNIT for "all compiled-in targets" (which obviously > as things stand is exactly one) seemed like a reasonable abstraction. > > Alternatively we could say that BITS_PER_UNIT is really part of the > definition of QImode and move it to the modes.def file. That makes sense to me - thus it will end up in insn-modes.h? Note that this file already uses BITS_PER_UNIT ... Richard. > Thanks, > Richard
Richard Biener <richard.guenther@gmail.com> writes: > On Sat, Dec 7, 2013 at 11:28 AM, Richard Sandiford > <rdsandiford@googlemail.com> wrote: >> Kenneth Zadeck <zadeck@naturalbridge.com> writes: >>>> + /* 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. >> >> FWIW it would be: >> >> temp = immed_wide_int_const (exp, TYPE_MODE (type)); >> >> But it's hard to tell whether it would buy much. It didn't show up as >> a hot spot when I was doing performance measurements before. >> >>>> --- 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. >> >> Sorry, I forgot yesterday an important detail behind why this seemed >> like a good thing. I think there was a strong feeling (from me and others) >> that wide-int.h shouldn't depend on tm.h. If we make wide-int.h depend >> on tm.h then basically all the compiler does. >> >> So as it stands we can't use BITS_PER_UNIT directly. Having a >> MAX_BITS_PER_UNIT for "all compiled-in targets" (which obviously >> as things stand is exactly one) seemed like a reasonable abstraction. >> >> Alternatively we could say that BITS_PER_UNIT is really part of the >> definition of QImode and move it to the modes.def file. > > That makes sense to me - thus it will end up in insn-modes.h? Yeah. > Note that this file already uses BITS_PER_UNIT ... Right, but only in the definitions that Kenny added in preparation for wide-int: 2013-03-28 Kenneth Zadeck <zadeck@naturalbridge.com> * genmodes.c (emit_max_int): New function. (emit_insn_modes_h): Added call to emit_max_function. * doc/rtl.texi (MAX_BITSIZE_MODE_ANY_INT, MAX_BITSIZE_MODE_ANY_MODE): Added doc. * machmode.def: Fixed comment. And it's those that we were changing to use MAX_BITS_PER_UNITS instead, so that there were no direct uses of BITS_PER_UNIT in insn-modes.h. In the original version wide-int.h had to include tm.h in order for these BITS_PER_UNIT-based macros to be usable. Thanks, Richard
Index: gcc/cselib.c =================================================================== --- gcc/cselib.c (revision 205749) +++ gcc/cselib.c (working copy) @@ -1121,11 +1121,8 @@ cselib_hash_rtx (rtx x, int create, enum 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); - } + for (i = 0; i < CONST_WIDE_INT_NUNITS (x); i++) + hash += CONST_WIDE_INT_ELT (x, i); return hash; case CONST_DOUBLE: Index: gcc/cse.c =================================================================== --- gcc/cse.c (revision 205749) +++ gcc/cse.c (working copy) @@ -2337,11 +2337,8 @@ hash_rtx_cb (const_rtx x, enum machine_m 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); - } + for (i = 0; i < CONST_WIDE_INT_NUNITS (x); i++) + hash += CONST_WIDE_INT_ELT (x, i); return hash; case CONST_DOUBLE: Index: gcc/rtl.h =================================================================== --- gcc/rtl.h (revision 205749) +++ gcc/rtl.h (working copy) @@ -348,7 +348,10 @@ struct GTY((chain_next ("RTX_NEXT (&%h)" unsigned return_val : 1; union { - /* RTXs are free to use up to 32 bit from here. */ + /* The final union field is aligned to 64 bits on LP64 hosts, + giving a 32-bit gap after the fields above. We optimize the + layout for that case and use the gap for extra code-specific + information. */ /* In a CONST_WIDE_INT (aka hwivec_def), this is the number of HOST_WIDE_INTs in the hwivec_def. */ Index: gcc/print-rtl.c =================================================================== --- gcc/print-rtl.c (revision 205749) +++ gcc/print-rtl.c (working copy) @@ -619,8 +619,7 @@ print_rtx (const_rtx in_rtx) break; case CONST_WIDE_INT: - if (! flag_simple) - fprintf (outfile, " "); + fprintf (outfile, " "); cwi_output_hex (outfile, in_rtx); break; #endif Index: gcc/expr.c =================================================================== --- gcc/expr.c (revision 205749) +++ gcc/expr.c (working copy) @@ -727,12 +727,11 @@ convert_modes (enum machine_mode mode, e if (mode == oldmode) return x; - if (CONST_SCALAR_INT_P (x) - && GET_MODE_CLASS (mode) == MODE_INT) + if (CONST_SCALAR_INT_P (x) && GET_MODE_CLASS (mode) == MODE_INT) { - /* 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 the caller did not tell us the old mode, then there is not + much to do with respect to canonicalization. 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), @@ -5295,10 +5294,10 @@ store_expr (tree exp, rtx target, int ca &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 or - CONST_WIDE_INT 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))) @@ -9477,19 +9476,18 @@ expand_expr_real_1 (tree exp, rtx target return decl_rtl; case INTEGER_CST: - { - tree type = TREE_TYPE (exp); - /* 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; - } + /* "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". */ + + temp = immed_wide_int_const (wide_int::from + (exp, + GET_MODE_PRECISION (TYPE_MODE (type)), + TYPE_SIGN (type)), + TYPE_MODE (type)); + return temp; + case VECTOR_CST: { tree tmp = NULL_TREE; @@ -11149,8 +11147,7 @@ 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_wide_int_const (elt, TYPE_MODE (TREE_TYPE (elt))); + RTVEC_ELT (v, i) = immed_wide_int_const (elt, inner); } return gen_rtx_CONST_VECTOR (mode, v);