Re: PATCH: named address space support (1/2: target-independent parts)

["Joseph S. Myers" <joseph at codesourcery dot com>]

On Wed, 20 Aug 2008, Ben Elliston wrote:

> I will post a follow-up patch that brings across the SPU backend
> changes.

This will be needed in order to review this first patch properly, as a 
requirement for review will be an adequate set of testcases, and those 
will I think necessarily be target-specific.

> @@ -4071,12 +4077,15 @@ grokdeclarator (const struct c_declarato
>  	pedwarn (OPT_pedantic, "duplicate %<restrict%>");
>        if (volatilep > 1)
>  	pedwarn (OPT_pedantic, "duplicate %<volatile%>");
> +      if (addr_space_p > 1)
> +	pedwarn (OPT_pedantic, "duplicate %qs", targetm.addr_space_name (TYPE_ADDR_SPACE (element_type)));

I don't think this is appropriate.  This is a pedwarn in the pedantic 
non-C99 case only, for duplicate qualifiers via typedef (direct duplicates 
are handled elsewhere).  The TR certainly does not consider pre-C99.  The 
correct checks are:

* If there are "qualifiers for two or more different address spaces", 
whether via typedefs or directly, give an error.

* For duplicates of the same qualifier, whether via typedefs or directly, 
allow them quietly.

with testcases for both of those (though if you don't have multiple named 
address spaces for any target at present, you won't be able to test the 
first).

> +		case csc_static:
> +		  error ("%qs combined with %<static%> qualifier for %qs", addrspace_name, name);
> +		  break;

Why are you disallowing address spaces together with static?  As far as I 
can tell the TR allows them.

Where do you check the constraint that a function type is not qualified 
with an address space qualifier?

> @@ -4846,6 +4905,10 @@ grokdeclarator (const struct c_declarato
>  
>  	type = c_build_qualified_type (type, type_quals);
>  
> +	if (POINTER_TYPE_P (type) && TYPE_ADDR_SPACE (type) && !extern_ref)
> +	  error ("%qs variable %qs must be extern", 
> +		 targetm.addr_space_name (TYPE_ADDR_SPACE (type)), name);

I don't see why such a check should be conditional on the type being a 
pointer type, and again I think initialized declarations and static 
declarations are valid.

> +  if (specs->address_space > 0)
> +    pedwarn (OPT_pedantic, "duplicate %qs", targetm.addr_space_name (specs->address_space));

The same point about duplicates as above applies: if a different address 
space then give an error, otherwise it is valid and so should not receive 
a pedwarn.

> +	  if (kind == C_ID_ADDRSPACE && !c_dialect_objc ())

Why the c_dialect_objc check?  If the feature is to be unsupported for 
Objective-C, a proper error in that case is needed.

> +	    {
> +	      declspecs_add_addrspace (specs, c_parser_peek_token (parser)->value);
> +	      c_parser_consume_token (parser);
> +	      attrs_ok = true;
> +	      seen_type = true;

I don't think setting seen_type here is correct; these are like qualifiers 
and qualifiers don't set seen_type.  The relevant testcase (that should go 
in the testsuite) would be an address-space qualifier followed by a 
typedef-name that was declared in an outer scope; without seen_type set 
the typedef-name will be handled as such, with seen_type set it will be 
treated as an identifier being redeclared in this scope as a variable.

> +	      /* If this operand is a pointer into another address
> +		 space, make the result of the comparison such a
> +		 pointer also.  */
> +	      if (OTHER_ADDR_SPACE_POINTER_TYPE_P (type0))
> +		{
> +		  int qual = ENCODE_QUAL_ADDR_SPACE (TYPE_ADDR_SPACE (TREE_TYPE (type0)));
> +		  result_type = build_pointer_type
> +		    (build_qualified_type (void_type_node, qual));
> +		}

For both places comparison operators are handled, the relevant check is 
not whether an address space is other than generic, it is whether the 
address spaces overlap.  If not, an error must be given.  If they do 
overlap, both must be converted to whichever is the larger address space 
(not whichever is the smaller).

Conditional expressions need similar handling.

I don't see any changes to convert_for_assignment to check the constraints 
on pointer assignment (and argument passing, function return, 
initialization) and address spaces.

> +@node Named Address Spaces
> +@section Named address spaces
> +@cindex named address spaces
> +
> +As an extension, the GNU C compiler supports named address spaces as
> +defined in the N1169 draft of ISO/IEC DTR 18037.  Support for named
> +address spaces in GCC will evolve as the draft technical report changes.
> +Calling conventions for any target might also change.  At present, only
> +the SPU target supports other address spaces.  On the SPU target, for
> +example, variables may be declared as belonging to another address space
> +by qualifying the type with the @var{__ea} address space identifier:

@var is for metasyntactic variables, not for programming language text 
(including names of programming language variables).  You mean 
@code{__ea}.

> +When the variable @var{i} is accessed, the compiler will generate

@code{i}.  And you need to define quite what the qualifier means, possibly 
with reference to an external SPU specification.

> +The @var{__ea} identifier may be used exactly like any other C type

@code{__ea}.

> +qualifier (e.g. const or volatile).  See the N1169 document for more

"e.g." needs to be followed by a comma or by "@:".  @code{const}, 
@code{volatile}.

>  @deftypefn {Target Hook} bool TARGET_SCALAR_MODE_SUPPORTED_P (enum machine_mode @var{mode})
> -Define this to return nonzero if the port is prepared to handle
> -insns involving scalar mode @var{mode}.  For a scalar mode to be
> -considered supported, all the basic arithmetic and comparisons
> -must work.
> +Define this to return nonzero if the port is prepared to handle insns
> +involving scalar mode @var{mode}.  For a scalar mode to be considered
> +supported, all the basic arithmetic and comparisons must work.

Don't mix reformatting this paragraph in with substantive unrelated 
changes to the compiler.

-- 
Joseph S. Myers
joseph@codesourcery.com