Re: [patch i386] Enable attribute based calling convention switching and support vaarg.

[Ralf Wildenhues <Ralf dot Wildenhues at gmx dot de>]

Hello Kai,

I have almost no knowledge about the specific code you're working on,
so please don't take the following general comments (plus obligatory
typo nits ;-) as a full-fledged review, but with some grains of salt:

* Kai Tietz wrote on Tue, Jul 01, 2008 at 02:19:01PM CEST:
> --- gcc.orig/gcc/config/i386/i386.c
> +++ gcc/gcc/config/i386/i386.c

> +/* This function returns the calling abi specific va_list type node.
> +   If FNDECL is NULL_TREE, it returns the current cfun specific
> +   va_list type, otherwise the the fndecl specific type.  */

s/the the/see the/  ?

> +tree
> +ix86_fn_abi_va_list (tree fndecl ATTRIBUTE_UNUSED)


> +/* This is used in function c_common_nodes_and_builtins ()
> +   to iterate through the target specific builtin types for va_list. The
> +   variable IDX is used as iterator. PNAME is a pointer
> +   to a 'const char *' and PTYPE is a pointer to a 'tree' type.
> +   The arguments PNAME and PTYPE are used to store the result and are set
> +   to the name of the va_list builtin type and its internal type.
> +   If the return value is zero, then there is no element for this index.
> +   Otherwise the IDX should be increased for the next call of this.
> +   Note, do not iterate a base builtins name like __builtin_va_list.  */
> +
> +int
> +ix86_enum_va_list (int idx, const char **pname, tree *ptree)

The comment before this function strikes me as odd, for several reasons:
First, it's longer than the function itself (not a problem in itself).
Then you start off with where this function is used, rather than what
the function does.  Isn't the most important bit of information what it
does?  Who is going to remember to adjust the comment if the function is
used in another dozen places later?  For denoting a function, it is not
really necessary to add 'function' before it, nor '()' afterwards (in
fact, the GNU Coding Standards recommend against the latter).

Next, there is no need to state the type of the arguments: that much is
clear from the function declaration.  PTYPE is a misnomer.  It seems to
me "builtins" in the last sentence is lacking an apostrophe.

Taken together, this is how I may have written the comment (beware, I
don't know exactly what this code actually does):

  /* Iterate through the target-specific builtin types for va_list.
     IDX denotes the iterator, *PTREE is set to the result type of
     the va_list builtin, and *PNAME to its internal type.
     Returns zero if there is no element for this index, otherwise
     IDX should be increased upon the next call.
     Note, do not iterate a base builtin's name like __builtin_va_list.
     Used from c_common_nodes_and_builtins.
  */

(Or even dropping the last sentence.)

> +  if (!TARGET_64BIT)
> +    return 0;
> +  switch (idx) {
> +  case 0:
> +    *ptree = ms_va_list_type_node;
> +    *pname = "__builtin_ms_va_list";
> +    break;
> +  case 1:
> +    *ptree = sysv_va_list_type_node;
> +    *pname = "__builtin_sysv_va_list";
> +    break;
> +  default:
> +    return 0;
> +  }
> +  return 1;
> +}
> +


> --- gcc.orig/gcc/tree-sra.c
> +++ gcc/gcc/tree-sra.c
> @@ -308,6 +308,26 @@ sra_type_can_be_decomposed_p (tree type)
>    return false;
>  }
>  
> +/* Returns true, if the TYPE is one of the avalable va_list types.

No need for comma.
s/avalable/available/

> +   Otherwise it returns false.
> +   Note, that for multiple calling conventions there can be more
> +   then just one va_list type be present.  */

s/then/than/
s/be present/present/

> --- gcc.orig/gcc/doc/tm.texi
> +++ gcc/gcc/doc/tm.texi
> @@ -4187,6 +4187,18 @@ This hook returns a type node for @code{

> +@defmac TARGET_ENUM_VA_LIST (@var{IDX}, @var{PNAME}, @var{PTYPE})

How come the @var names are written in caps?  Several more instances
below.

> +This target macro is used in function @code{c_common_nodes_and_builtins}
> +to iterate through the target specific builtin types for va_list.

Hmm.  Here, the description (which is similar to the one before
ix86_enum_va_list) strikes me as better fitting, and I see that 
a similar style is used elsewhere in the manual.  Still, listing
first where some macro is used rather than what it does, strikes
me as odd, unless that one specific usage is the only one possible.

> The
> +variable @var{IDX} is used as iterator. @var{PNAME} has to be a pointer
> +to a @code{const char *} and @var{PTYPE} a pointer to a @code{tree} typed
> +variable.
> +The arguments @var{PNAME} and @var{PTYPE} are used to store the result of
> +this macro and are set to the name of the va_list builtin type and its
> +internal type.
> +If the return value of this macro is zero, then there is no more element.
> +Otherwise the @var{IDX} should be increased for the next call of this
> +macro to iterate through all types. $$$$

The $$$$ looks spurious to me.

> +@end defmac
> +
>  @defmac APPLY_RESULT_SIZE
>  Define this macro if @samp{untyped_call} and @samp{untyped_return}
>  need more space than is implied by @code{FUNCTION_VALUE_REGNO_P} for

Cheers,
Ralf