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


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

Re: [patch]: Add support of new __int128 type for targets having 128-bit integer scalar support


On Mon, 10 May 2010, Kai Tietz wrote:

> +	tree complex_int128_type_node;

Do we actually need this node (and associated changes in various places) 
at all?

> @@ -2988,6 +3001,9 @@ c_common_type_for_mode (enum machine_mod
>  
>        if (mode == TYPE_MODE (complex_integer_type_node) && !unsignedp)
>  	return complex_integer_type_node;
> +      if (complex_int128_type_node
> +          && mode == TYPE_MODE (complex_int128_type_node) && !unsignedp)
> +	return complex_int128_type_node;
>  
>        inner_mode = GET_MODE_INNER (mode);
>        inner_type = c_common_type_for_mode (inner_mode, unsignedp);

This code has special handling for complex float, double, long double and 
int.  It then has generic handling for the modes for other complex types.  
The global trees likewise have just those complex types.  My guess is that 
this is because those are the most common complex types (and maybe there's 
some reason to want unique trees for the standard ones).  There is no such 
global tree or special handling for complex unsigned int or complex long 
long, for example - and since complex __int128 is surely more obscure than 
those, I don't think there should be a special global tree node or 
associated code for it; it should just be handled like e.g. complex char, 
and a tree node built as required.  If there is something that breaks, 
then I'd expect similar breakage for the other types, and that global 
trees would need adding for them.

>  	  switch (i)
>  	    {
> +	    case RID_INT128:
> +	      if (int128_integer_type_node == NULL_TREE)
> +	        {
> +		  error_at (loc, "%<__int128%> is not supported for this target");
> +		  return specs;
> +		}
> +	      if (!in_system_header)
> +		pedwarn (loc, OPT_pedantic,
> +			 "ISO C does not support %<__int128%> type");
> +
> +	      if (specs->long_long_p)
> +		error_at (loc,
> +			  ("both %<__int128%> and %<long long%> in "
> +			   "declaration specifiers"));
> +	      else if (specs->long_p)
> +		error_at (loc,
> +			  ("both %<__int128%> and %<long%> in "
> +			   "declaration specifiers"));

If long_long_p is true so will be specs->long_p, so the long_long_p check 
is redundant.

You need a check in this case for specs->saturating_p to catch _Sat 
__int128 (you have a check above for the other ordering, __int128 _Sat).

I don't see any problems with the C front-end changes beyond the issues I 
comment on above.

> +As an extension the integer scalar type @code{__int128} is supported for
> +targets having an integer mode wide enough to hold 128-bit.
> +Simply write @code{__int128} for a signed 128-bit integer, or
> +@code{unsigned __int128} for an unsigned 128-bit integer.  There is no
> +support in gcc to express an integer constant of type @code{__int128}
> +for targets having @code{long long} integer with less then 128 bit width.

GCC, not gcc.

> Index: gcc/gcc/testsuite/gcc.dg/int128-1.c
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ gcc/gcc/testsuite/gcc.dg/int128-1.c	2010-05-10 13:32:27.323368800 +0200
> @@ -0,0 +1,28 @@
> +/* { dg-do run { target int128 } } */
> +/* { dg-options "-std=gnu99" } */
> +
> +#include <stdarg.h>
> +
> +extern void abort(void);
> +
> +void foo(int i, ...)
> +{
> +  __int128 q;
> +  va_list va;
> +
> +  va_start(va, i);
> +  q = va_arg(va, __int128);
> +  va_end(va);
> +
> +  if (q != 5)
> +    abort();
> +}
> +
> +int main(void)
> +{
> +  __int128 q = 5;

It would be good to test a bit-pattern that actually puts nonzero data in 
each word, in case of bugs that pass only some words correctly.  E.g. 
((__int128)5 | ((__int128)6 << 32) | ((__int128)7 << 64) | ((__int128)8 << 96)).

> Index: gcc/gcc/testsuite/gcc.dg/int128-2.c
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ gcc/gcc/testsuite/gcc.dg/int128-2.c	2010-05-10 13:32:27.323368800 +0200
> @@ -0,0 +1,21 @@
> +/* { dg-do run { target int128 } } */
> +/* { dg-options "-std=gnu99" } */
> +
> +extern void abort(void);
> +
> +__int128 foo (signed __int128 a, unsigned __int128 b)
> +{
> +  return (__int128) (a + (__int128) b);
> +}
> +
> +int main(void)
> +{
> +  __int128 q = 5;
> +  unsigned __int128 r = 1;
> +
> +  q = foo (q, r);
> +  if (q != 6)

Similarly, test a more interesting case of arithmetic (you can build up 
arbitrary constants by pieces as above), and also test other arithmetic 
operations?  With there being little testing of the runtime support for 
128-bit integers there could easily be bugs, so I'd advise at least one 
test of each operation ISO C allows on integers, using numbers that aren't 
representable in 64 bits (signed or unsigned).  When I added tests for 
integer/floating-point conversions I found those involving TImode were 
completely broken (bug 25028 - which I fixed for most targets and which 
may since have been fixed for the remaining targets although it's not 
closed).

-- 
Joseph S. Myers
joseph@codesourcery.com


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