Re: __intN patch 3/5: main __int128 -> __intN conversion.
DJ Delorie writes: >> FAIL: g++.dg/init/enum1.C -std=gnu++11 (test for errors, line 12) >> FAIL: g++.dg/init/enum1.C -std=gnu++1y (test for errors, line 12) >> FAIL: g++.dg/init/enum1.C -std=gnu++98 (test for errors, line 12) >> >> That used to complain about "enum1.C:12:1: error: no integral type can >> represent all of the enumerator values for 'test'" > > On what host? It works OK on x86-64. Have you considered doing proper testing? http://gcc.gnu.org/ml/gcc-testresults/2014-10/msg02106.html Andreas. -- Andreas Schwab, SUSE Labs, sch...@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different."
Re: __intN patch 3/5: main __int128 -> __intN conversion.
> FAIL: g++.dg/init/enum1.C -std=gnu++11 (test for errors, line 12) > FAIL: g++.dg/init/enum1.C -std=gnu++1y (test for errors, line 12) > FAIL: g++.dg/init/enum1.C -std=gnu++98 (test for errors, line 12) > > That used to complain about "enum1.C:12:1: error: no integral type can > represent all of the enumerator values for 'test'" On what host? It works OK on x86-64. PASS: g++.dg/init/enum1.C -std=gnu++98 (test for errors, line 12) PASS: g++.dg/init/enum1.C -std=gnu++98 (test for excess errors) PASS: g++.dg/init/enum1.C -std=gnu++11 (test for errors, line 12) PASS: g++.dg/init/enum1.C -std=gnu++11 (test for excess errors) PASS: g++.dg/init/enum1.C -std=gnu++1y (test for errors, line 12) PASS: g++.dg/init/enum1.C -std=gnu++1y (test for excess errors)
Re: __intN patch 3/5: main __int128 -> __intN conversion.
> This is okay. Thanks! Committed.
Re: __intN patch 3/5: main __int128 -> __intN conversion.
On Thu, Oct 16, 2014 at 1:48 AM, Markus Trippelsdorf
wrote:
> On 2014.10.15 at 17:00 -0400, DJ Delorie wrote:
>>
>> > If you could implement the second option, it would be appreciated.
>>
>> Could you please test this for me? It builds as a powerpc-elf
>> cross-compiler (at least the host half) but I don't have a power
>> machine here to test on.
>>
>> Index: rs6000-c.c
>> ===
>> --- rs6000-c.c(revision 216241)
>> +++ rs6000-c.c(working copy)
>> @@ -157,12 +157,29 @@ init_vector_keywords (void)
>> {
>>__int128_type = get_identifier ("__int128_t");
>>__uint128_type = get_identifier ("__uint128_t");
>> }
>> }
>>
>> +/* Helper function to find out which RID_INT_N_* code is the one for
>> + __int128, if any. Returns RID_MAX+1 if none apply, which is safe
>> + (for our purposes, since we always expect to have __int128) to
>> + compare against. */
>> +static int
>> +rid_int128(void)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < NUM_INT_N_ENTS; i ++)
>> +if (int_n_enabled_p[i]
>> + && int_n_data[i].bitsize == 128)
>> + return RID_INT_N_0 + i;
>> +
>> + return RID_MAX + 1;
>> +}
>> +
>> /* Called to decide whether a conditional macro should be expanded.
>> Since we have exactly one such macro (i.e, 'vector'), we do not
>> need to examine the 'tok' parameter. */
>>
>> static cpp_hashnode *
>> rs6000_macro_to_expand (cpp_reader *pfile, const cpp_token *tok)
>> @@ -231,13 +248,13 @@ rs6000_macro_to_expand (cpp_reader *pfil
>>
>> if (rid_code == RID_UNSIGNED || rid_code == RID_LONG
>> || rid_code == RID_SHORT || rid_code == RID_SIGNED
>> || rid_code == RID_INT || rid_code == RID_CHAR
>> || rid_code == RID_FLOAT
>> || (rid_code == RID_DOUBLE && TARGET_VSX)
>> - || (rid_code == RID_INT128 && TARGET_VADDUQM))
>> + || (rid_code == rid_int128 () && TARGET_VADDUQM))
>> {
>> expand_this = C_CPP_HASHNODE (__vector_keyword);
>> /* If the next keyword is bool or pixel, it
>>will need to be expanded as well. */
>> do
>> tok = cpp_peek_token (pfile, idx++);
>>
>
> Testing went fine (although there is a lot of noise because of the
> -std=gnu11 standard change).
> But I cannot approve the patch. Adding more people to CC.
This is okay.
There are POWER machines in the GCC Compile Farm, which should have
been used to test the original patch and at least could have been used
to test the fix.
Thanks, David
Re: __intN patch 3/5: main __int128 -> __intN conversion.
FAIL: g++.dg/init/enum1.C -std=gnu++11 (test for errors, line 12) FAIL: g++.dg/init/enum1.C -std=gnu++1y (test for errors, line 12) FAIL: g++.dg/init/enum1.C -std=gnu++98 (test for errors, line 12) That used to complain about "enum1.C:12:1: error: no integral type can represent all of the enumerator values for 'test'" Andreas. -- Andreas Schwab, SUSE Labs, sch...@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different."
Re: __intN patch 3/5: main __int128 -> __intN conversion.
On 2014.10.15 at 17:00 -0400, DJ Delorie wrote:
>
> > If you could implement the second option, it would be appreciated.
>
> Could you please test this for me? It builds as a powerpc-elf
> cross-compiler (at least the host half) but I don't have a power
> machine here to test on.
>
> Index: rs6000-c.c
> ===
> --- rs6000-c.c(revision 216241)
> +++ rs6000-c.c(working copy)
> @@ -157,12 +157,29 @@ init_vector_keywords (void)
> {
>__int128_type = get_identifier ("__int128_t");
>__uint128_type = get_identifier ("__uint128_t");
> }
> }
>
> +/* Helper function to find out which RID_INT_N_* code is the one for
> + __int128, if any. Returns RID_MAX+1 if none apply, which is safe
> + (for our purposes, since we always expect to have __int128) to
> + compare against. */
> +static int
> +rid_int128(void)
> +{
> + int i;
> +
> + for (i = 0; i < NUM_INT_N_ENTS; i ++)
> +if (int_n_enabled_p[i]
> + && int_n_data[i].bitsize == 128)
> + return RID_INT_N_0 + i;
> +
> + return RID_MAX + 1;
> +}
> +
> /* Called to decide whether a conditional macro should be expanded.
> Since we have exactly one such macro (i.e, 'vector'), we do not
> need to examine the 'tok' parameter. */
>
> static cpp_hashnode *
> rs6000_macro_to_expand (cpp_reader *pfile, const cpp_token *tok)
> @@ -231,13 +248,13 @@ rs6000_macro_to_expand (cpp_reader *pfil
>
> if (rid_code == RID_UNSIGNED || rid_code == RID_LONG
> || rid_code == RID_SHORT || rid_code == RID_SIGNED
> || rid_code == RID_INT || rid_code == RID_CHAR
> || rid_code == RID_FLOAT
> || (rid_code == RID_DOUBLE && TARGET_VSX)
> - || (rid_code == RID_INT128 && TARGET_VADDUQM))
> + || (rid_code == rid_int128 () && TARGET_VADDUQM))
> {
> expand_this = C_CPP_HASHNODE (__vector_keyword);
> /* If the next keyword is bool or pixel, it
>will need to be expanded as well. */
> do
> tok = cpp_peek_token (pfile, idx++);
>
Testing went fine (although there is a lot of noise because of the
-std=gnu11 standard change).
But I cannot approve the patch. Adding more people to CC.
Thanks.
--
Markus
Re: __intN patch 3/5: main __int128 -> __intN conversion.
> If you could implement the second option, it would be appreciated.
Could you please test this for me? It builds as a powerpc-elf
cross-compiler (at least the host half) but I don't have a power
machine here to test on.
Index: rs6000-c.c
===
--- rs6000-c.c (revision 216241)
+++ rs6000-c.c (working copy)
@@ -157,12 +157,29 @@ init_vector_keywords (void)
{
__int128_type = get_identifier ("__int128_t");
__uint128_type = get_identifier ("__uint128_t");
}
}
+/* Helper function to find out which RID_INT_N_* code is the one for
+ __int128, if any. Returns RID_MAX+1 if none apply, which is safe
+ (for our purposes, since we always expect to have __int128) to
+ compare against. */
+static int
+rid_int128(void)
+{
+ int i;
+
+ for (i = 0; i < NUM_INT_N_ENTS; i ++)
+if (int_n_enabled_p[i]
+ && int_n_data[i].bitsize == 128)
+ return RID_INT_N_0 + i;
+
+ return RID_MAX + 1;
+}
+
/* Called to decide whether a conditional macro should be expanded.
Since we have exactly one such macro (i.e, 'vector'), we do not
need to examine the 'tok' parameter. */
static cpp_hashnode *
rs6000_macro_to_expand (cpp_reader *pfile, const cpp_token *tok)
@@ -231,13 +248,13 @@ rs6000_macro_to_expand (cpp_reader *pfil
if (rid_code == RID_UNSIGNED || rid_code == RID_LONG
|| rid_code == RID_SHORT || rid_code == RID_SIGNED
|| rid_code == RID_INT || rid_code == RID_CHAR
|| rid_code == RID_FLOAT
|| (rid_code == RID_DOUBLE && TARGET_VSX)
- || (rid_code == RID_INT128 && TARGET_VADDUQM))
+ || (rid_code == rid_int128 () && TARGET_VADDUQM))
{
expand_this = C_CPP_HASHNODE (__vector_keyword);
/* If the next keyword is bool or pixel, it
will need to be expanded as well. */
do
tok = cpp_peek_token (pfile, idx++);
Re: __intN patch 3/5: main __int128 -> __intN conversion.
On Wed, 15 Oct 2014, Paolo Carlini wrote:
A couple of small issues in std/limits and in the testsuite are causing
regressions which I'm fixing with the below.
- max() _GLIBCXX_USE_NOEXCEPT { return __glibcxx_max_b (TYPE, BITSIZE);; } \
+ max() _GLIBCXX_USE_NOEXCEPT { return __glibcxx_max_b (TYPE, BITSIZE);; } \
Maybe shorten the double ;; as well?
--
Marc Glisse
Re: __intN patch 3/5: main __int128 -> __intN conversion.
Hi,
On 10/14/2014 09:44 PM, DJ Delorie wrote:
extensions. Is this OK? If so, is there anything else, or can I
check the whole mess in yet?
Go ahead.
Thanks! Committed.
A couple of small issues in std/limits and in the testsuite are causing
regressions which I'm fixing with the below.
Thanks,
Paolo.
//
2014-10-15 Paolo Carlini
* include/std/limits: Remove stray spaces after backslash.
* testsuite/20_util/declval/requirements/1_neg.cc: Adjust dg-error
line number.
* testsuite/20_util/make_signed/requirements/typedefs_neg.cc:
Likewise.
* testsuite/20_util/make_unsigned/requirements/typedefs_neg.cc:
Likewise.
Index: include/std/limits
===
--- include/std/limits (revision 216237)
+++ include/std/limits (working copy)
@@ -1422,7 +1422,7 @@
min() _GLIBCXX_USE_NOEXCEPT { return __glibcxx_min_b (TYPE, BITSIZE); }
\
\
static _GLIBCXX_CONSTEXPR TYPE
\
- max() _GLIBCXX_USE_NOEXCEPT { return __glibcxx_max_b (TYPE, BITSIZE);; }
\
+ max() _GLIBCXX_USE_NOEXCEPT { return __glibcxx_max_b (TYPE, BITSIZE);; }
\
\
static _GLIBCXX_USE_CONSTEXPR int digits
\
= BITSIZE - 1;
\
Index: testsuite/20_util/declval/requirements/1_neg.cc
===
--- testsuite/20_util/declval/requirements/1_neg.cc (revision 216237)
+++ testsuite/20_util/declval/requirements/1_neg.cc (working copy)
@@ -19,7 +19,7 @@
// with this library; see the file COPYING3. If not see
//
Re: __intN patch 3/5: main __int128 -> __intN conversion.
On 2014.10.14 at 17:10 -0400, DJ Delorie wrote: > > > ../../gcc/gcc/config/rs6000/rs6000-c.c:237:24: error: ‘RID_INT128’ was not > > declared in this scope > > Two options: > > 1. If you know the RS6000 will never have any __intN other than >__int128, just use RID_INT_N_0, although this is a hack it will >work as long as there *is* an __int128 for RS6000. > > 2. Alternately, you need to check all entries in the __int array >for proper size, which is more correct but more complex. > > Would you like me to work on the second option, or would you prefer to > tackle this yourself? If you could implement the second option, it would be appreciated. (I just noticed the build failure on the POWER7 compile farm machine, that I use for testing sometimes.) Thanks. -- Markus
Re: __intN patch 3/5: main __int128 -> __intN conversion.
> ../../gcc/gcc/config/rs6000/rs6000-c.c:237:24: error: ‘RID_INT128’ was not > declared in this scope Two options: 1. If you know the RS6000 will never have any __intN other than __int128, just use RID_INT_N_0, although this is a hack it will work as long as there *is* an __int128 for RS6000. 2. Alternately, you need to check all entries in the __int array for proper size, which is more correct but more complex. Would you like me to work on the second option, or would you prefer to tackle this yourself?
Re: __intN patch 3/5: main __int128 -> __intN conversion.
On 2014.08.25 at 23:03 -0400, DJ Delorie wrote: > > > I'd like to see the updated version of the whole of patch 3 (tested > > to be actually independent of the other patches) for review, though > > I won't be reviewing the C++ parts. > > Here it is. Tested on x86_64. I include the msp430-modes.def patch > for demonstration purposes although obviously msp430's __int20 won't > work without the other patches. This patch breaks ppc64: ../../gcc/gcc/config/rs6000/rs6000-c.c: In function ‘cpp_hashnode* rs6000_macro_to_expand(cpp_reader*, const cpp_token*)’: ../../gcc/gcc/config/rs6000/rs6000-c.c:237:24: error: ‘RID_INT128’ was not declared in this scope make[3]: *** [rs6000-c.o] Error 1 -- Markus
Re: __intN patch 3/5: main __int128 -> __intN conversion.
> > extensions. Is this OK? If so, is there anything else, or can I > > check the whole mess in yet? > > Go ahead. Thanks! Committed.
Re: __intN patch 3/5: main __int128 -> __intN conversion.
On 10/13/2014 04:54 PM, DJ Delorie wrote: This is what I ended up with for the test case. It was a bit tricky since it only works with msp430x (not msp430) and requires the gnu extensions. Is this OK? If so, is there anything else, or can I check the whole mess in yet? Go ahead. Jason
Re: __intN patch 3/5: main __int128 -> __intN conversion.
This is what I ended up with for the test case. It was a bit tricky
since it only works with msp430x (not msp430) and requires the gnu
extensions. Is this OK? If so, is there anything else, or can I
check the whole mess in yet?
// { dg-do compile { target msp430*-*-* } }
// { dg-options "-std=gnu++11" }
// { dg-skip-if "" { msp430*-*-* } { "-mcpu=msp430" } { "" } }
__int20 x;
__int20 foo (__int20 a, unsigned __int20 b)
{
return a + b;
}
// { dg-final { scan-assembler "\n_?_Z3foou5int20u6uint20\[: \t\n\]" } }
Re: __intN patch 3/5: main __int128 -> __intN conversion.
OK, thanks. Jason
Re: __intN patch 3/5: main __int128 -> __intN conversion.
> > if (same_type_p (TYPE_MAIN_VARIANT (t1), long_long_unsigned_type_node)
> > || same_type_p (TYPE_MAIN_VARIANT (t2),
> > long_long_unsigned_type_node))
> > return build_type_attribute_variant (long_long_unsigned_type_node,
> > attributes);
>
> Your patch only compares t1/t2 to int_n_trees[i].signed_type.
Checking the code before this logic, we can make some assumptions:
/* Both real or both integers; use the one with greater precision. */
* we can assume the two types have the same precision
/* The types are the same; no need to do anything fancy. */
* we can assume the two types are different
In the case of non-128 __intN, checking only for signed *should* work
- the only other type that's the same precision but a different type
*is* the unsigned variant (enforced in toplev.c).
Even in the case of __int128, if either type is signed __int128 and
the other type is different AT ALL, the result will be unsigned
__int128 if either type is unsigned, or signed __int128.
But a new patch which checks everything anyway is attached :-)
Index: typeck.c
===
--- typeck.c(revision 216012)
+++ typeck.c(working copy)
@@ -270,6 +270,7 @@
enum tree_code code1 = TREE_CODE (t1);
enum tree_code code2 = TREE_CODE (t2);
tree attributes;
+ int i;
/* In what follows, we slightly generalize the rules given in [expr] so
@@ -364,17 +365,6 @@
: long_long_integer_type_node);
return build_type_attribute_variant (t, attributes);
}
- if (int128_integer_type_node != NULL_TREE
- && (same_type_p (TYPE_MAIN_VARIANT (t1),
- int128_integer_type_node)
- || same_type_p (TYPE_MAIN_VARIANT (t2),
- int128_integer_type_node)))
- {
- tree t = ((TYPE_UNSIGNED (t1) || TYPE_UNSIGNED (t2))
- ? int128_unsigned_type_node
- : int128_integer_type_node);
- return build_type_attribute_variant (t, attributes);
- }
/* Go through the same procedure, but for longs. */
if (same_type_p (TYPE_MAIN_VARIANT (t1), long_unsigned_type_node)
@@ -388,6 +378,26 @@
? long_unsigned_type_node : long_integer_type_node);
return build_type_attribute_variant (t, attributes);
}
+
+ /* For __intN types, either the type is __int128 (and is lower
+priority than the types checked above, but higher than other
+128-bit types) or it's known to not be the same size as other
+types (enforced in toplev.c). Prefer the unsigned type. */
+ for (i = 0; i < NUM_INT_N_ENTS; i ++)
+ {
+ if (int_n_enabled_p [i]
+ && (same_type_p (TYPE_MAIN_VARIANT (t1),
int_n_trees[i].signed_type)
+ || same_type_p (TYPE_MAIN_VARIANT (t2),
int_n_trees[i].signed_type)
+ || same_type_p (TYPE_MAIN_VARIANT (t1),
int_n_trees[i].unsigned_type)
+ || same_type_p (TYPE_MAIN_VARIANT (t2),
int_n_trees[i].unsigned_type)))
+ {
+ tree t = ((TYPE_UNSIGNED (t1) || TYPE_UNSIGNED (t2))
+ ? int_n_trees[i].unsigned_type
+ : int_n_trees[i].signed_type);
+ return build_type_attribute_variant (t, attributes);
+ }
+ }
+
/* Otherwise prefer the unsigned one. */
if (TYPE_UNSIGNED (t1))
return build_type_attribute_variant (t1, attributes);
Re: __intN patch 3/5: main __int128 -> __intN conversion.
> > Otherwise, I don't see what moving the test would accomplish. If
> > "long" is never 128 bits, it doesn't matter if the int128 test is
> > before or after it, and the other intN are never the same size as
> > standard types,
>
> I don't see how you can assert that these will never happen.
It's checked specifically in toplev.c - if the backend *does* request
a type that matches a standard size, they won't get it:
+static bool
+standard_type_bitsize (int bitsize)
+{
+ /* As a special exception, we always want __int128 enabled if possible. */
+ if (bitsize == 128)
+return false;
+ if (bitsize == CHAR_TYPE_SIZE
+ || bitsize == SHORT_TYPE_SIZE
+ || bitsize == INT_TYPE_SIZE
+ || bitsize == LONG_TYPE_SIZE
+ || (bitsize == LONG_LONG_TYPE_SIZE && LONG_LONG_TYPE_SIZE <
GET_MODE_BITSIZE (TImode)))
+return true;
+ return false;
+}
+ /* This must happen after the backend has a chance to process
+command line options, but before the parsers are
+initialized. */
+ for (i = 0; i < NUM_INT_N_ENTS; i ++)
+ if (targetm.scalar_mode_supported_p (int_n_data[i].m)
+ && ! standard_type_bitsize (int_n_data[i].bitsize)
+ && int_n_data[i].bitsize <= HOST_BITS_PER_WIDE_INT * 2)
+ int_n_enabled_p[i] = true;
+ else
+ int_n_enabled_p[i] = false;
+
Re: __intN patch 3/5: main __int128 -> __intN conversion.
On 10/03/2014 04:11 PM, DJ Delorie wrote: Note that there is a separate __int128_t type that isn't part of the "standard" extension. Maybe it's there for that type? Otherwise, I don't see what moving the test would accomplish. If "long" is never 128 bits, it doesn't matter if the int128 test is before or after it, and the other intN are never the same size as standard types, I don't see how you can assert that these will never happen. Why not make the code more correct, while we're touching it? and we already have code to prefer unsigned for intN types. What I don't see is the intN parallel to this: if (same_type_p (TYPE_MAIN_VARIANT (t1), long_long_unsigned_type_node) || same_type_p (TYPE_MAIN_VARIANT (t2), long_long_unsigned_type_node)) return build_type_attribute_variant (long_long_unsigned_type_node, attributes); Your patch only compares t1/t2 to int_n_trees[i].signed_type. Jason
Re: __intN patch 3/5: main __int128 -> __intN conversion.
Note that there is a separate __int128_t type that isn't part of the "standard" extension. Maybe it's there for that type? Otherwise, I don't see what moving the test would accomplish. If "long" is never 128 bits, it doesn't matter if the int128 test is before or after it, and the other intN are never the same size as standard types, and we already have code to prefer unsigned for intN types.
Re: __intN patch 3/5: main __int128 -> __intN conversion.
On 10/02/2014 02:00 PM, DJ Delorie wrote: Ah, good point. In which case I don't see what this code is trying to accomplish relative to falling through to the "prefer the unsigned one" code below. Shall we just remove it? I don't know for sure. There was __int128 code there, I replaced it with the "same" code, so as to avoid any functional differences on mainstream targets. I imagine the code is there for when __int128 is the same size as some other types besides long long. But if __int128 happened to be the same size as long the code was wrong. Well, I suppose it could be there to prefer __int128 to intTI_type_node. I guess let's move the intN handling below the code for (u)long and add support for unsigned extended integers like there is for both long and long long. Jason
Re: __intN patch 3/5: main __int128 -> __intN conversion.
> > The test would only pass for msp430x (and fail for msp430, which is > > the same target back-end). Do I need to redo the big patch, or would > > a separate one suffice? > > Separate is fine. Turns out it's mangled like this: __int20 foo (__int20 a, unsigned __int20 b); _Z3foou5int20u6uint20
Re: __intN patch 3/5: main __int128 -> __intN conversion.
> Ah, good point. In which case I don't see what this code is trying to > accomplish relative to falling through to the "prefer the unsigned one" > code below. Shall we just remove it? I don't know for sure. There was __int128 code there, I replaced it with the "same" code, so as to avoid any functional differences on mainstream targets. I imagine the code is there for when __int128 is the same size as some other types besides long long.
Re: __intN patch 3/5: main __int128 -> __intN conversion.
On 10/02/2014 12:41 PM, DJ Delorie wrote: The C++ standard says that extended integer types participate in the usual arithmetic conversions. If I add a 32-bit int and an __int48, the usual arithmetic conversions should convert the int to __int48. Except the code you're referring to isn't part of that conversion. It only handles types that are the same size, not different sizes Ah, good point. In which case I don't see what this code is trying to accomplish relative to falling through to the "prefer the unsigned one" code below. Shall we just remove it? The test would only pass for msp430x (and fail for msp430, which is the same target back-end). Do I need to redo the big patch, or would a separate one suffice? Separate is fine. Jason
Re: __intN patch 3/5: main __int128 -> __intN conversion.
> The C++ standard says that extended integer types participate in the > usual arithmetic conversions. If I add a 32-bit int and an __int48, the > usual arithmetic conversions should convert the int to __int48. Except the code you're referring to isn't part of that conversion. It only handles types that are the same size, not different sizes, and as I said the __intN types are never the same size as standard types (except __int128) so there are no conversions to worry about - in the code you're referring to. In the common parts of the MI the __intN types are handled separately, in each case where i_t[] is referenced. > We still need mangling tests for __int20, though. The test would only pass for msp430x (and fail for msp430, which is the same target back-end). Do I need to redo the big patch, or would a separate one suffice?
Re: __intN patch 3/5: main __int128 -> __intN conversion.
On 10/02/2014 10:22 AM, Jason Merrill wrote: Extended integer types larger than long long also participate in enumeral promotion if needed, but I think your changes to c_common_type_for_size handle that. ...and it seems we currently don't support enums with underlying type larger than long long, though that would be nice to fix and adding extended integer types to integer_types[] would fix it. Jason
Re: __intN patch 3/5: main __int128 -> __intN conversion.
On 10/01/2014 11:52 PM, DJ Delorie wrote: It seems like the int128 code here was broken and this is continuing that brokenness. Extended integer types have integer conversion rank corresponding to their bitsize, so int128 should have higher rank than long long, but here it was being checked after long long, and your code also follows the long long code. Also, we should be checking for both signed and unsigned variants. In this case, we know that the two types are the same bitsize. True, when __int128 is the same size as long long it has lower rank. But it isn't always the same size; specifically, in the x32 ABI long long is 64 bits. So we definitely need to deal with extended integer types larger than long long. If you plan to allow __intN with sizes between those of int and long long, they need to have the appropriate intermediate conversion rank for their size. But... we don't know the bit sizes at the gcc source level, we only know them by iterating the array. We'd have to iterate the array at evey step to see if it's between bitsize N and M, or add the standard types to some array, or add the __intN variants to integer_types[] and re-sort i_t[]. Right. However, doing that introduces the __intN types to *everything* that uses integer_types[]. I was hesitant to add nonstandard types to the standard promotion rules. The C++ standard says that extended integer types participate in the usual arithmetic conversions. If I add a 32-bit int and an __int48, the usual arithmetic conversions should convert the int to __int48. I don't mind if you deal with this by asserting that extended integer types never fall between int and long long rather than actually trying to support it. Extended integer types larger than long long also participate in enumeral promotion if needed, but I think your changes to c_common_type_for_size handle that. Basically I think the integral conversion code in cp_common_type ought to be rewritten to work on integer_types rather than naming specific types. That would be the "re-sort it" option. - 'n', /* itk_int128 */ - 'o', /* itk_unsigned_int128 */ + /* __intN types are handled separately */ Where are they mangled now? I also don't see any mangling tests. They mangle as Inn like I20 or I128 (that might be the wrong letter, but the syntax is like that). It surprised me that there was a case for "other unknown types", but there's a case for it :-) Ah, I see that 'n' and 'o' for __int128 are handled further down in write_builtin_type. And we do have mangling tests for that (g++.dg/abi/mangle43.C). We still need mangling tests for __int20, though. Jason
Re: __intN patch 3/5: main __int128 -> __intN conversion.
> It seems like the int128 code here was broken and this is continuing > that brokenness. Extended integer types have integer conversion rank > corresponding to their bitsize, so int128 should have higher rank than > long long, but here it was being checked after long long, and your code > also follows the long long code. Also, we should be checking for both > signed and unsigned variants. In this case, we know that the two types are the same bitsize. We also know that no __intN type has the same size as a standard type (__int128 being an exception). So in any case where other __intN types match, they won't match any of the other tests in this area anyway. Aside from choosing unsigned vs sighed (which the code does), there are no other conversions within those bitsizes. So if you think the __int128 case should be elsewhere in that logic (and it seems to me that "long long" *should* be preferred over __int128 if they're ths same bitsize) I can move it, but I don't think there's any need to worry about other __intN types here. > If you plan to allow __intN with sizes between those of int and long > long, they need to have the appropriate intermediate conversion rank for > their size. But... we don't know the bit sizes at the gcc source level, we only know them by iterating the array. We'd have to iterate the array at evey step to see if it's between bitsize N and M, or add the standard types to some array, or add the __intN variants to integer_types[] and re-sort i_t[]. However, doing that introduces the __intN types to *everything* that uses integer_types[]. I was hesitant to add nonstandard types to the standard promotion rules. > Basically I think the integral conversion code in cp_common_type ought > to be rewritten to work on integer_types rather than naming specific types. That would be the "re-sort it" option. > > - 'n', /* itk_int128 */ > > - 'o', /* itk_unsigned_int128 */ > > + /* __intN types are handled separately */ > > Where are they mangled now? I also don't see any mangling tests. They mangle as Inn like I20 or I128 (that might be the wrong letter, but the syntax is like that). It surprised me that there was a case for "other unknown types", but there's a case for it :-) The NULs are there just to ensure the array access works, and to tell the code that there isn't a special case for those types.
Re: __intN patch 3/5: main __int128 -> __intN conversion.
On 09/30/2014 07:14 PM, DJ Delorie wrote:
Could one of you two please review the remaining C++ parts (cp/*) ?
https://gcc.gnu.org/ml/gcc-patches/2014-08/msg02360.html
+ for (i = 0; i < NUM_INT_N_ENTS; i ++)
{
+ if (int_n_enabled_p [i]
+ && (same_type_p (TYPE_MAIN_VARIANT (t1),
+ int_n_trees[i].signed_type)
+ || same_type_p (TYPE_MAIN_VARIANT (t2),
+ int_n_trees[i].signed_type)))
+ {
+ tree t = ((TYPE_UNSIGNED (t1) || TYPE_UNSIGNED (t2))
+ ? int_n_trees[i].unsigned_type
+ : int_n_trees[i].signed_type);
+ return build_type_attribute_variant (t, attributes);
+ }
}
It seems like the int128 code here was broken and this is continuing
that brokenness. Extended integer types have integer conversion rank
corresponding to their bitsize, so int128 should have higher rank than
long long, but here it was being checked after long long, and your code
also follows the long long code. Also, we should be checking for both
signed and unsigned variants.
If you plan to allow __intN with sizes between those of int and long
long, they need to have the appropriate intermediate conversion rank for
their size.
Basically I think the integral conversion code in cp_common_type ought
to be rewritten to work on integer_types rather than naming specific types.
- 'n', /* itk_int128 */
- 'o', /* itk_unsigned_int128 */
+ /* __intN types are handled separately */
Where are they mangled now? I also don't see any mangling tests.
Jason
Re: __intN patch 3/5: main __int128 -> __intN conversion.
Joseph S. Myers wrote: > The non-C++/libstdc++ parts are OK with those changes. Jonathan Wakely wrote: > These libstdc++ changes are OK for trunk. Jason/Nathan, Could one of you two please review the remaining C++ parts (cp/*) ? https://gcc.gnu.org/ml/gcc-patches/2014-08/msg02360.html Thanks!
Re: __intN patch 3/5: main __int128 -> __intN conversion.
On 30/09/14 15:37 -0400, DJ Delorie wrote: Joseph S. Myers wrote: The non-C++/libstdc++ parts are OK with those changes. Jonathan Wakely wrote: >* libstdc++-v3/ >* src/c++11/limits.cc: Add support for __intN types. >* include/std/type_traits: Likewise. >* include/std/limits: Likewise. >* include/c_std/cstdlib: Likewise. >* include/bits/cpp_type_traits.h: Likewise. >* include/c_global/cstdlib: Likewise. These libstdc++ changes are OK for trunk. Do I still need approval for gcc/cp/* or do these cover it? I can't approve gcc/cp/* changes and as Joseph says non-C++ I think you need to chase someone else (i.e. Jason :-) for that.
Re: __intN patch 3/5: main __int128 -> __intN conversion.
Joseph S. Myers wrote: > The non-C++/libstdc++ parts are OK with those changes. Jonathan Wakely wrote: > >* libstdc++-v3/ > > * src/c++11/limits.cc: Add support for __intN types. > > * include/std/type_traits: Likewise. > > * include/std/limits: Likewise. > > * include/c_std/cstdlib: Likewise. > > * include/bits/cpp_type_traits.h: Likewise. > > * include/c_global/cstdlib: Likewise. > > These libstdc++ changes are OK for trunk. Do I still need approval for gcc/cp/* or do these cover it?
Re: __intN patch 3/5: main __int128 -> __intN conversion.
On 29/09/14 14:06 -0400, DJ Delorie wrote: Just one question about the include/std/limits changes below. It seems that __glibcxx_signed_b isn't strictly necessary as it doesn't use the B argument, so is it just there for consistency? Yup. OK, thanks for confirming.
Re: __intN patch 3/5: main __int128 -> __intN conversion.
> Just one question about the include/std/limits changes below. > > It seems that __glibcxx_signed_b isn't strictly necessary as it > doesn't use the B argument, so is it just there for consistency? Yup.
Re: __intN patch 3/5: main __int128 -> __intN conversion.
On 25/08/14 23:03 -0400, DJ Delorie wrote: I'd like to see the updated version of the whole of patch 3 (tested to be actually independent of the other patches) for review, though I won't be reviewing the C++ parts. Here it is. Tested on x86_64. I include the msp430-modes.def patch for demonstration purposes although obviously msp430's __int20 won't work without the other patches. [snip] * libstdc++-v3/ * src/c++11/limits.cc: Add support for __intN types. * include/std/type_traits: Likewise. * include/std/limits: Likewise. * include/c_std/cstdlib: Likewise. * include/bits/cpp_type_traits.h: Likewise. * include/c_global/cstdlib: Likewise. These libstdc++ changes are OK for trunk. Just one question about the include/std/limits changes below. It seems that __glibcxx_signed_b isn't strictly necessary as it doesn't use the B argument, so is it just there for consistency? Index: libstdc++-v3/include/std/limits === --- libstdc++-v3/include/std/limits (revision 214383) +++ libstdc++-v3/include/std/limits (working copy) @@ -122,27 +122,38 @@ #ifndef __glibcxx_long_double_tinyness_before # define __glibcxx_long_double_tinyness_before false #endif // You should not need to define any macros below this point. -#define __glibcxx_signed(T)((T)(-1) < 0) +#define __glibcxx_signed_b(T,B)((T)(-1) < 0) -#define __glibcxx_min(T) \ - (__glibcxx_signed (T) ? -__glibcxx_max (T) - 1 : (T)0) +#define __glibcxx_min_b(T,B) \ + (__glibcxx_signed_b (T,B) ? -__glibcxx_max_b (T,B) - 1 : (T)0) -#define __glibcxx_max(T) \ - (__glibcxx_signed (T) ? \ - (T)1 << (__glibcxx_digits (T) - 1)) - 1) << 1) + 1) : ~(T)0) +#define __glibcxx_max_b(T,B) \ + (__glibcxx_signed_b (T,B) ? \ + (T)1 << (__glibcxx_digits_b (T,B) - 1)) - 1) << 1) + 1) : ~(T)0) -#define __glibcxx_digits(T) \ - (sizeof(T) * __CHAR_BIT__ - __glibcxx_signed (T)) +#define __glibcxx_digits_b(T,B)\ + (B - __glibcxx_signed_b (T,B)) // The fraction 643/2136 approximates log10(2) to 7 significant digits. +#define __glibcxx_digits10_b(T,B) \ + (__glibcxx_digits_b (T,B) * 643L / 2136) + +#define __glibcxx_signed(T) \ + __glibcxx_signed_b (T, sizeof(T) * __CHAR_BIT__)
Re: __intN patch 3/5: main __int128 -> __intN conversion.
On Mon, 25 Aug 2014, DJ Delorie wrote:
> + for (i = 0; i < NUM_INT_N_ENTS; i ++)
> +if (int_n_enabled_p[i])
> + {
> + char buf[35+20+20];
> +
> + /* These are used to configure the C++ library. */
> +
> + if (!flag_iso || int_n_data[i].bitsize == POINTER_SIZE)
> + {
> + sprintf (buf, "__GLIBCXX_TYPE_INT_N_%d=__int%d", i,
> int_n_data[i].bitsize);
> + cpp_define (parse_in, buf);
> +
> + sprintf (buf, "__GLIBCXX_BITSIZE_INT_N_%d=%d", i,
> int_n_data[i].bitsize);
> + cpp_define (parse_in, buf);
> + }
> + }
I think this should at least initially be conditioned on c_dialect_cxx ().
> + case RID_INT_N_0:
> + case RID_INT_N_1:
> + case RID_INT_N_2:
> + case RID_INT_N_3:
> + specs->int_n_idx = i - RID_INT_N_0;
> + if (!in_system_header_at (input_location)
> + /* As a special exception, allow a type that's used
> + for __SIZE_TYPE__. */
> + && int_n_data[specs->int_n_idx].bitsize != POINTER_SIZE)
Given the precedent for long long as __SIZE_TYPE__, I don't think we
should have that special exception.
The non-C++/libstdc++ parts are OK with those changes.
--
Joseph S. Myers
jos...@codesourcery.com
Re: __intN patch 3/5: main __int128 -> __intN conversion.
> I don't see flag_iso as relevant here (since the macros are in the > implementation namespace). The definitions could reasonably be restricted > to c_dialect_cxx (), though, given that they are specifically for use by > libstdc++ (and it's easier to add a macro later for C if needed, than to > remove one after adding it). They reflect a previous check in libstdc++ to remove __int128 specializations for non-ISO cases. Now, though, libstdc++ doesn't know which types are needed for pointers and which aren't, so the check was moved to inside gcc, which does know.
Re: __intN patch 3/5: main __int128 -> __intN conversion.
On Fri, 22 Aug 2014, DJ Delorie wrote: > > > > Maybe you need to refactor __glibcxx_digits so there is a version > > > > taking > > > > the bitsize as an argument rather than using sizeof(T) * __CHAR_BIT__, > > > > but > > > > that should be the only change needed to handle such types with the > > > > existing macros. The bitsize macros should be the only ones needing > > > > predefining to pass information to libstdc++. > > > > > > Like this? > > > > Yes (well, the libstdc++ changes will need to go to the libstdc++ mailing > > list for review there, but this is the sort of thing I'd expect to keep > > the way libstdc++ defines these limits as consistent as possible between > > different types). > > Ok, here's the updated c-cppbuiltins.c and all the libstdc++-v3 > changes, cross-posted to the libstdc++ list. I tested the macros on > x86-64 (before and after) and msp430 (after) with __int128 and __int20 > and get the right values in all cases. I'd like to see the updated version of the whole of patch 3 (tested to be actually independent of the other patches) for review, though I won't be reviewing the C++ parts. > + if (!flag_iso || int_n_data[i].bitsize == POINTER_SIZE) I don't see flag_iso as relevant here (since the macros are in the implementation namespace). The definitions could reasonably be restricted to c_dialect_cxx (), though, given that they are specifically for use by libstdc++ (and it's easier to add a macro later for C if needed, than to remove one after adding it). -- Joseph S. Myers jos...@codesourcery.com
Re: __intN patch 3/5: main __int128 -> __intN conversion.
> > > Maybe you need to refactor __glibcxx_digits so there is a version taking
> > > the bitsize as an argument rather than using sizeof(T) * __CHAR_BIT__,
> > > but
> > > that should be the only change needed to handle such types with the
> > > existing macros. The bitsize macros should be the only ones needing
> > > predefining to pass information to libstdc++.
> >
> > Like this?
>
> Yes (well, the libstdc++ changes will need to go to the libstdc++ mailing
> list for review there, but this is the sort of thing I'd expect to keep
> the way libstdc++ defines these limits as consistent as possible between
> different types).
Ok, here's the updated c-cppbuiltins.c and all the libstdc++-v3
changes, cross-posted to the libstdc++ list. I tested the macros on
x86-64 (before and after) and msp430 (after) with __int128 and __int20
and get the right values in all cases.
Index: gcc/c-family/c-cppbuiltin.c
===
--- gcc/c-family/c-cppbuiltin.c (revision 213886)
+++ gcc/c-family/c-cppbuiltin.c (working copy)
@@ -775,12 +775,14 @@ cpp_iec_559_complex_value (void)
}
/* Hook that registers front end and target-specific built-ins. */
void
c_cpp_builtins (cpp_reader *pfile)
{
+ int i;
+
/* -undef turns off target-specific built-ins. */
if (flag_undef)
return;
define_language_independent_builtin_macros (pfile);
@@ -845,12 +847,29 @@ c_cpp_builtins (cpp_reader *pfile)
builtin_define_type_minmax ("__WCHAR_MIN__", "__WCHAR_MAX__",
underlying_wchar_type_node);
builtin_define_type_minmax ("__WINT_MIN__", "__WINT_MAX__", wint_type_node);
builtin_define_type_max ("__PTRDIFF_MAX__", ptrdiff_type_node);
builtin_define_type_max ("__SIZE_MAX__", size_type_node);
+ for (i = 0; i < NUM_INT_N_ENTS; i ++)
+if (int_n_enabled_p[i])
+ {
+ char buf[35+20+20];
+
+ /* These are used to configure the C++ library. */
+
+ if (!flag_iso || int_n_data[i].bitsize == POINTER_SIZE)
+ {
+ sprintf (buf, "__GLIBCXX_TYPE_INT_N_%d=__int%d", i,
int_n_data[i].bitsize);
+ cpp_define (parse_in, buf);
+
+ sprintf (buf, "__GLIBCXX_BITSIZE_INT_N_%d=%d", i,
int_n_data[i].bitsize);
+ cpp_define (parse_in, buf);
+ }
+ }
+
/* stdint.h and the testsuite need to know these. */
builtin_define_stdint_macros ();
/* Provide information for library headers to determine whether to
define macros such as __STDC_IEC_559__ and
__STDC_IEC_559_COMPLEX__. */
@@ -993,15 +1012,20 @@ c_cpp_builtins (cpp_reader *pfile)
else if (flag_stack_protect == 1)
cpp_define (pfile, "__SSP__=1");
if (flag_openmp)
cpp_define (pfile, "_OPENMP=201307");
- if (int128_integer_type_node != NULL_TREE)
-builtin_define_type_sizeof ("__SIZEOF_INT128__",
- int128_integer_type_node);
+ for (i = 0; i < NUM_INT_N_ENTS; i ++)
+if (int_n_enabled_p[i])
+ {
+ char buf[15+20];
+ sprintf(buf, "__SIZEOF_INT%d__", int_n_data[i].bitsize);
+ builtin_define_type_sizeof (buf,
+ int_n_trees[i].signed_type);
+ }
builtin_define_type_sizeof ("__SIZEOF_WCHAR_T__", wchar_type_node);
builtin_define_type_sizeof ("__SIZEOF_WINT_T__", wint_type_node);
builtin_define_type_sizeof ("__SIZEOF_PTRDIFF_T__",
unsigned_ptrdiff_type_node);
/* A straightforward target hook doesn't work, because of problems
Index: libstdc++-v3/src/c++11/limits.cc
===
--- libstdc++-v3/src/c++11/limits.cc(revision 213886)
+++ libstdc++-v3/src/c++11/limits.cc(working copy)
@@ -385,60 +385,72 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
const bool numeric_limits::is_bounded;
const bool numeric_limits::is_modulo;
const bool numeric_limits::traps;
const bool numeric_limits::tinyness_before;
const float_round_style numeric_limits::round_style;
-#if !defined(__STRICT_ANSI__) && defined(_GLIBCXX_USE_INT128)
- const bool numeric_limits<__int128>::is_specialized;
- const int numeric_limits<__int128>::digits;
- const int numeric_limits<__int128>::digits10;
- const int numeric_limits<__int128>::max_digits10;
- const bool numeric_limits<__int128>::is_signed;
- const bool numeric_limits<__int128>::is_integer;
- const bool numeric_limits<__int128>::is_exact;
- const int numeric_limits<__int128>::radix;
- const int numeric_limits<__int128>::min_exponent;
- const int numeric_limits<__int128>::min_exponent10;
- const int numeric_limits<__int128>::max_exponent;
- const int numeric_limits<__int128>::max_exponent10;
- const bool numeric_limits<__int128>::has_infinity;
- const bool numeric_limits<__int128>::has_quiet_NaN;
- const bool numeric_limits<__int128>::has_signaling_NaN;
- const float_denorm_style numeric_limits<__int128>::has_denorm;
- const bool
Re: __intN patch 3/5: main __int128 -> __intN conversion.
On Fri, 22 Aug 2014, DJ Delorie wrote: > > Maybe you need to refactor __glibcxx_digits so there is a version taking > > the bitsize as an argument rather than using sizeof(T) * __CHAR_BIT__, but > > that should be the only change needed to handle such types with the > > existing macros. The bitsize macros should be the only ones needing > > predefining to pass information to libstdc++. > > Like this? Yes (well, the libstdc++ changes will need to go to the libstdc++ mailing list for review there, but this is the sort of thing I'd expect to keep the way libstdc++ defines these limits as consistent as possible between different types). -- Joseph S. Myers jos...@codesourcery.com
Re: __intN patch 3/5: main __int128 -> __intN conversion.
> Maybe you need to refactor __glibcxx_digits so there is a version taking > the bitsize as an argument rather than using sizeof(T) * __CHAR_BIT__, but > that should be the only change needed to handle such types with the > existing macros. The bitsize macros should be the only ones needing > predefining to pass information to libstdc++. Like this? #define __glibcxx_signed_b(T,B) ((T)(-1) < 0) #define __glibcxx_min_b(T,B)\ (__glibcxx_signed_b (T,B) ? -__glibcxx_max_b (T,B) - 1 : (T)0) #define __glibcxx_max_b(T,B)\ (__glibcxx_signed_b (T,B) ? \ (T)1 << (__glibcxx_digits_b (T,B) - 1)) - 1) << 1) + 1) : ~(T)0) #define __glibcxx_digits_b(T,B) \ (B - __glibcxx_signed_b (T,B)) // The fraction 643/2136 approximates log10(2) to 7 significant digits. #define __glibcxx_digits10_b(T,B) \ (__glibcxx_digits_b (T,B) * 643L / 2136) #define __glibcxx_signed(T) \ __glibcxx_signed_b (T, sizeof(T) * __CHAR_BIT__) #define __glibcxx_min(T) \ __glibcxx_min (T, sizeof(T) * __CHAR_BIT_) #define __glibcxx_max(T) \ __glibcxx_max (T, sizeof(T) * __CHAR_BIT_) #define __glibcxx_digits(T) \ __glibcxx_digits (T, sizeof(T) * __CHAR_BIT_) #define __glibcxx_digits10(T) \ __glibcxx_digits10 (T, sizeof(T) * __CHAR_BIT_)
Re: __intN patch 3/5: main __int128 -> __intN conversion.
On Thu, 21 Aug 2014, DJ Delorie wrote: > > Maybe you need to refactor __glibcxx_digits so there is a version > > taking the bitsize as an argument rather than using sizeof(T) * > > __CHAR_BIT__, but that should be the only change needed to handle > > such types with the existing macros. > > Since the other macros use this macro, we'd need a complete second set > of macros just for the __intN types anyway, each of which takes a Well, the existing macros would be defined in terms of the new ones. > bitsize and passes it down. Since gcc already knows all the right > answers for the __intN types and needs to emit other macros for them > anyway, where's the benefit? The more predefined macros there are, the more impact on startup time (though I don't have any specific figures). -- Joseph S. Myers jos...@codesourcery.com
Re: __intN patch 3/5: main __int128 -> __intN conversion.
> I don't see any corresponding HOST_BITS_PER_WIDE_INT test for __int128 > being removed (and anyway HOST_BITS_PER_WIDE_INT is now always 64, so such > a test for __int128 would be dead code). It was there when I started the patch, honest! :-) Removed ;-) > > For each __int we need to provide an __INT_MIN__ and > > __INT_MAX__, just like for "char" we provide __CHAR_MIN__ and > > __CHAR_MAX__. > > No, those are provided for use by , which only covers standard C > types (and in particular does not cover __int128). Ok, removed. > Maybe you need to refactor __glibcxx_digits so there is a version > taking the bitsize as an argument rather than using sizeof(T) * > __CHAR_BIT__, but that should be the only change needed to handle > such types with the existing macros. Since the other macros use this macro, we'd need a complete second set of macros just for the __intN types anyway, each of which takes a bitsize and passes it down. Since gcc already knows all the right answers for the __intN types and needs to emit other macros for them anyway, where's the benefit? Also, consider that the specialization for all the other types is duplicated in full, I'm already using less source code for my 1..4 types than the library would have used for its 1..4 types ;-)
Re: __intN patch 3/5: main __int128 -> __intN conversion.
On Thu, 21 Aug 2014, DJ Delorie wrote: > > > + /* This must happen after the backend has a chance to process > > > + command line options, but before the parsers are > > > + initialized. */ > > > + for (i = 0; i < NUM_INT_N_ENTS; i ++) > > > + if (targetm.scalar_mode_supported_p (int_n_data[i].m) > > > + && ! standard_type_bitsize (int_n_data[i].bitsize) > > > + && int_n_data[i].bitsize <= HOST_BITS_PER_WIDE_INT * 2) > > > + int_n_enabled_p[i] = true; > > > > This HOST_BITS_PER_WIDE_INT * 2 check seems wrong. > > That check was there before, for __int128, I left it as-is. There is no > __int128 (currently) if it's bigger then HBPWI*2. I don't see any corresponding HOST_BITS_PER_WIDE_INT test for __int128 being removed (and anyway HOST_BITS_PER_WIDE_INT is now always 64, so such a test for __int128 would be dead code). > > All this block of code appears to be new rather than replacing any > > existing code doing something similar with __int128. As such, I think > > it's best considered separately from the main __intN support. > > For each __int we need to provide an __INT_MIN__ and > __INT_MAX__, just like for "char" we provide __CHAR_MIN__ and > __CHAR_MAX__. No, those are provided for use by , which only covers standard C types (and in particular does not cover __int128). > > Some of this may be needed for libstdc++, but not all. As far as I can > > tell, the existing __glibcxx_min, __glibcxx_max, __glibcxx_digits, > > __glibcxx_digits10, __glibcxx_max_digits10 macros can be used in most > > cases and avoid any need to predefine macros for the min or max of __intN; > > you only need to communicate which types exist and their sizes in bits > > (that is, a single macro predefine for each N, with anything else being > > considered separately if otherwise thought desirable). > > I tried that, and wasn't able to get a simpler macro to do it inline > than the full macro that let gcc figure out the values. Consider the > two N of 20 and 128; one is not a multiple of bytes and the other will > likely stress any runtime math. If __intN is supported, GCC needs to be able to handle folding arithmetic on it, such as the expansion of the existing __glibcxx_max macro. Maybe you need to refactor __glibcxx_digits so there is a version taking the bitsize as an argument rather than using sizeof(T) * __CHAR_BIT__, but that should be the only change needed to handle such types with the existing macros. The bitsize macros should be the only ones needing predefining to pass information to libstdc++. -- Joseph S. Myers jos...@codesourcery.com
Re: __intN patch 3/5: main __int128 -> __intN conversion.
> Why are types only entered in integer_types if wider than long long?
IIRC it was so that TImode (__int128) could get into the array (it was
there before) without adding the other smaller types, which (I think)
don't need to be in there. I don't recall why they're left out,
but... ah, I remember. ITK has to be sorted by bitsize and it's used
for type promotion, we don't want types promoted *to* the __intN
types, just *from* them.
> > +static bool
> > +standard_type_bitsize (int bitsize)
> > +{
> > + /* As a special exception, we always want __int128 enabled if possible.
> > */
> > + if (bitsize == 128)
> > +return false;
> > + if (bitsize == CHAR_TYPE_SIZE
> > + || bitsize == SHORT_TYPE_SIZE
> > + || bitsize == INT_TYPE_SIZE
> > + || bitsize == LONG_TYPE_SIZE
> > + || (bitsize == LONG_LONG_TYPE_SIZE && LONG_LONG_TYPE_SIZE <
> > GET_MODE_BITSIZE (TImode)))
>
> I don't think there should be a special case depending on the size of long
> long here.
I think it was from before I had the special case for "bitsize == 128". I'll
remove it.
> > + /* This must happen after the backend has a chance to process
> > +command line options, but before the parsers are
> > +initialized. */
> > + for (i = 0; i < NUM_INT_N_ENTS; i ++)
> > + if (targetm.scalar_mode_supported_p (int_n_data[i].m)
> > + && ! standard_type_bitsize (int_n_data[i].bitsize)
> > + && int_n_data[i].bitsize <= HOST_BITS_PER_WIDE_INT * 2)
> > + int_n_enabled_p[i] = true;
>
> This HOST_BITS_PER_WIDE_INT * 2 check seems wrong.
That check was there before, for __int128, I left it as-is. There is no
__int128 (currently) if it's bigger then HBPWI*2.
> > mode_for_size (unsigned int size, enum mode_class mclass, int limit)
> > {
> > - enum machine_mode mode;
> > + enum machine_mode mode = BLKmode;
> > + int i;
>
> I don't see any need for this initialization to be added.
Removed.
> >bprecision
> > -= MIN (precision + BITS_PER_UNIT_LOG + 1, MAX_FIXED_MODE_SIZE);
> > += MIN (precision, MAX_FIXED_MODE_SIZE);
>
> This change doesn't make sense to me - if it is needed, I think it should
> be separated out, not included in this patch which should simply about
> providing generic __intN support in the cases where __int128 is currently
> supported.
Perhaps is belongs in the 1/5 patch, where I removed lots of "assume
all types are powers-of-two sizes" logic? I split up the patch and
logs by hand from a master patch, I'm not surprised I got a few
mis-placed.
> > @@ -1157,13 +1165,13 @@ enum omp_clause_map_kind
> >OMP_CLAUSE_MAP_ALLOC,
> >OMP_CLAUSE_MAP_TO,
> >OMP_CLAUSE_MAP_FROM,
> >OMP_CLAUSE_MAP_TOFROM,
> >/* The following kind is an internal only map kind, used for pointer
> > based
> > array sections. OMP_CLAUSE_SIZE for these is not the pointer size,
> > - which is implicitly POINTER_SIZE / BITS_PER_UNIT, but the bias. */
> > + which is implicitly POINTER_SIZE_UNITS, but the bias. */
>
> POINTER_SIZE_UNITS changes belong in another patch, not this one.
Again, probably the 1/5 patch.
> >/* ptr_type_node can't be used here since ptr_mode is only set when
> > toplev calls backend_init which is not done with -E or pch. */
> > - psize = POINTER_SIZE / BITS_PER_UNIT;
> > + psize = POINTER_SIZE_UNITS;
>
> Likewise.
Likewise :-)
> > @@ -844,12 +846,47 @@ c_cpp_builtins (cpp_reader *pfile)
> >builtin_define_type_max ("__LONG_LONG_MAX__",
> > long_long_integer_type_node);
> >builtin_define_type_minmax ("__WCHAR_MIN__", "__WCHAR_MAX__",
> > underlying_wchar_type_node);
> >builtin_define_type_minmax ("__WINT_MIN__", "__WINT_MAX__",
> > wint_type_node);
> >builtin_define_type_max ("__PTRDIFF_MAX__", ptrdiff_type_node);
> >builtin_define_type_max ("__SIZE_MAX__", size_type_node);
> > + for (i = 0; i < NUM_INT_N_ENTS; i ++)
> > +if (int_n_enabled_p[i])
> > + {
> > + char buf[35+20+20];
> > + char buf_min[15+20];
> > + char buf_max[15+20];
> > +
> > + sprintf(buf_min, "__INT%d_MIN__", int_n_data[i].bitsize);
> > + sprintf(buf_max, "__INT%d_MAX__", int_n_data[i].bitsize);
>
> All this block of code appears to be new rather than replacing any
> existing code doing something similar with __int128. As such, I think
> it's best considered separately from the main __intN support.
For each __int we need to provide an __INT_MIN__ and
__INT_MAX__, just like for "char" we provide __CHAR_MIN__ and
__CHAR_MAX__.
> Also note missing spaces before '(' in this code.
Fixed.
> Some of this may be needed for libstdc++, but not all. As far as I can
> tell, the existing __glibcxx_min, __glibcxx_max, __glibcxx_digits,
> __glibcxx_digits10, __glibcxx_max_digits10 macros can be used in most
> cases and avoid any need to predefine macros for the min or max of __intN;
> you only need to communicate which types exist and their sizes in bits
> (that is, a single
Re: __intN patch 3/5: main __int128 -> __intN conversion.
On Wed, 13 Aug 2014, DJ Delorie wrote:
> + for (i = 0; i < NUM_INT_N_ENTS; i ++)
> +{
> + int_n_trees[i].signed_type = make_signed_type (int_n_data[i].bitsize);
> + int_n_trees[i].unsigned_type = make_unsigned_type
> (int_n_data[i].bitsize);
> + TYPE_SIZE (int_n_trees[i].signed_type) = bitsize_int
> (int_n_data[i].bitsize);
> + TYPE_SIZE (int_n_trees[i].unsigned_type) = bitsize_int
> (int_n_data[i].bitsize);
> +
> + if (int_n_data[i].bitsize > LONG_LONG_TYPE_SIZE)
> + {
> + integer_types[itk_intN_0 + i * 2] = int_n_trees[i].signed_type;
> + integer_types[itk_unsigned_intN_0 + i * 2] =
> int_n_trees[i].unsigned_type;
> + }
> +}
Why are types only entered in integer_types if wider than long long?
> +static bool
> +standard_type_bitsize (int bitsize)
> +{
> + /* As a special exception, we always want __int128 enabled if possible. */
> + if (bitsize == 128)
> +return false;
> + if (bitsize == CHAR_TYPE_SIZE
> + || bitsize == SHORT_TYPE_SIZE
> + || bitsize == INT_TYPE_SIZE
> + || bitsize == LONG_TYPE_SIZE
> + || (bitsize == LONG_LONG_TYPE_SIZE && LONG_LONG_TYPE_SIZE <
> GET_MODE_BITSIZE (TImode)))
I don't think there should be a special case depending on the size of long
long here.
> + /* This must happen after the backend has a chance to process
> + command line options, but before the parsers are
> + initialized. */
> + for (i = 0; i < NUM_INT_N_ENTS; i ++)
> + if (targetm.scalar_mode_supported_p (int_n_data[i].m)
> + && ! standard_type_bitsize (int_n_data[i].bitsize)
> + && int_n_data[i].bitsize <= HOST_BITS_PER_WIDE_INT * 2)
> + int_n_enabled_p[i] = true;
This HOST_BITS_PER_WIDE_INT * 2 check seems wrong. wide-int should allow
integer types bigger than HOST_BITS_PER_WIDE_INT * 2 to be supported; it's
the job of targetm.scalar_mode_supported_p to return the correct result
(not claiming to support such types if the target doesn't have the
required wide-int support or is missing anything else required), not for
target-independent code to override it. I don't see any definitions of
that hook that obviously get this wrong.
> mode_for_size (unsigned int size, enum mode_class mclass, int limit)
> {
> - enum machine_mode mode;
> + enum machine_mode mode = BLKmode;
> + int i;
I don't see any need for this initialization to be added.
>bprecision
> -= MIN (precision + BITS_PER_UNIT_LOG + 1, MAX_FIXED_MODE_SIZE);
> += MIN (precision, MAX_FIXED_MODE_SIZE);
This change doesn't make sense to me - if it is needed, I think it should
be separated out, not included in this patch which should simply about
providing generic __intN support in the cases where __int128 is currently
supported.
> @@ -1157,13 +1165,13 @@ enum omp_clause_map_kind
>OMP_CLAUSE_MAP_ALLOC,
>OMP_CLAUSE_MAP_TO,
>OMP_CLAUSE_MAP_FROM,
>OMP_CLAUSE_MAP_TOFROM,
>/* The following kind is an internal only map kind, used for pointer based
> array sections. OMP_CLAUSE_SIZE for these is not the pointer size,
> - which is implicitly POINTER_SIZE / BITS_PER_UNIT, but the bias. */
> + which is implicitly POINTER_SIZE_UNITS, but the bias. */
POINTER_SIZE_UNITS changes belong in another patch, not this one.
>/* ptr_type_node can't be used here since ptr_mode is only set when
> toplev calls backend_init which is not done with -E or pch. */
> - psize = POINTER_SIZE / BITS_PER_UNIT;
> + psize = POINTER_SIZE_UNITS;
Likewise.
> @@ -844,12 +846,47 @@ c_cpp_builtins (cpp_reader *pfile)
>builtin_define_type_max ("__LONG_LONG_MAX__", long_long_integer_type_node);
>builtin_define_type_minmax ("__WCHAR_MIN__", "__WCHAR_MAX__",
> underlying_wchar_type_node);
>builtin_define_type_minmax ("__WINT_MIN__", "__WINT_MAX__",
> wint_type_node);
>builtin_define_type_max ("__PTRDIFF_MAX__", ptrdiff_type_node);
>builtin_define_type_max ("__SIZE_MAX__", size_type_node);
> + for (i = 0; i < NUM_INT_N_ENTS; i ++)
> +if (int_n_enabled_p[i])
> + {
> + char buf[35+20+20];
> + char buf_min[15+20];
> + char buf_max[15+20];
> +
> + sprintf(buf_min, "__INT%d_MIN__", int_n_data[i].bitsize);
> + sprintf(buf_max, "__INT%d_MAX__", int_n_data[i].bitsize);
All this block of code appears to be new rather than replacing any
existing code doing something similar with __int128. As such, I think
it's best considered separately from the main __intN support. Also note
missing spaces before '(' in this code.
Some of this may be needed for libstdc++, but not all. As far as I can
tell, the existing __glibcxx_min, __glibcxx_max, __glibcxx_digits,
__glibcxx_digits10, __glibcxx_max_digits10 macros can be used in most
cases and avoid any need to predefine macros for the min or max of __intN;
you only need to communicate which types exist and their sizes in bits
(that is, a single macro predefine for each N, w
Re: __intN patch 3/5: main __int128 -> __intN conversion.
> A while ago I've removed a couple of those 'typedef struct' things, as > they are not required in C++ anymore. Is there any particular reason > why this couldn't be simply 'struct int_n_data_t' ? No particular reason.
Re: __intN patch 3/5: main __int128 -> __intN conversion.
On Wed, 2014-08-13 at 18:11 -0400, DJ Delorie wrote:
> This is the main __int128 -> __intN conversion patch. We support up
> to four __int types per target, which may include __int128. I
> include the minimal change to the msp430 backend to illustrate how to
> add a target-specific __int type.
>
> [...]
> Index: gcc/machmode.h
> ===
> --- gcc/machmode.h(revision 213886)
> +++ gcc/machmode.h(working copy)
> @@ -342,7 +342,19 @@ extern void init_adjust_machine_modes (v
>GET_MODE_PRECISION (MODE2))
>
> #define HWI_COMPUTABLE_MODE_P(MODE) \
>(SCALAR_INT_MODE_P (MODE) \
> && GET_MODE_PRECISION (MODE) <= HOST_BITS_PER_WIDE_INT)
>
> +typedef struct {
> + /* These parts are initailized by genmodes output */
> + unsigned int bitsize;
> + enum machine_mode m;
> + /* RID_* is RID_INTN_BASE + index into this array */
> +} int_n_data_t;
> +
A while ago I've removed a couple of those 'typedef struct' things, as
they are not required in C++ anymore. Is there any particular reason
why this couldn't be simply 'struct int_n_data_t' ?
Cheers,
Oleg
