Re: [PATCH] Stage 2 projects - AltiVec rewrite

[Richard Henderson <rth at redhat dot com>]

On Thu, Apr 28, 2005 at 10:14:31AM +0200, Paolo Bonzini wrote:
> @@ -5999,8 +5999,12 @@ resolve_overloaded_builtin (tree functio
>        }
>  
>      default:
> -      return NULL;
> +      if (targetm.resolve_overloaded_builtin
> +	  && DECL_BUILT_IN_CLASS (function) == BUILT_IN_MD)
> +	return targetm.resolve_overloaded_builtin (function, params);

You've removed the BUILT_IN_NORMAL check and not replaced it.
This new code is in the wrong place, since this default should
be shadowed by the BUILT_IN_NORMAL check.

> +struct altivec_builtin_types
> +{
> +  enum rs6000_builtins code;
> +  enum rs6000_builtins overloaded_code;
> +  signed char ret_type;
> +  signed char op1;
> +  signed char op2;
> +  signed char op3;
> +};

This structure should go down with its use.

> +static tree altivec_build_resolved_builtin (tree *, tree *, int,
> +				    const struct altivec_builtin_types *);

You'll say its needed for this decl, but this decl isn't needed due
to function ordering.

> +static inline tree rs6000_builtin_type (int);
> +static inline bool rs6000_builtin_type_compatible (tree, int);

Indeed, I make it a habit not to pre-declare anything unless forced.
I prefer functions to simply be ordered such that the decls aren't
needed.

> +const struct altivec_builtin_types altivec_overloaded_builtins[] = {

Not checked.

> +/* Convert a type stored into a struct altivec_builtin_types as ID,
> +   into a tree.  The types are in rs6000_builtin_types: negative values
> +   create a pointer type for the type associated to -ID.  */

Is it certain that ID 0 doesn't need a pointer?  You could just
as easily make it ~ID instead...

> +  if (TREE_CODE (t) == INTEGER_TYPE
>	 && TREE_CODE (builtin_type) == INTEGER_TYPE)

INTEGER_TYPE only?  What about enums and bool?  Do you not want these
being promoted?  I'd have thought INTEGRAL_TYPE_P would be more appropriate.

> +      /* Remove the const from the pointers to simplify the overload
> +	 matching further down.  */
> +      if (POINTER_TYPE_P (decl_type)
> +	  && POINTER_TYPE_P (type)
> +	  && TYPE_QUALS (TREE_TYPE (type)) != 0)
> +	{
> +          if (TYPE_READONLY (TREE_TYPE (type))
> +	      && !TYPE_READONLY (TREE_TYPE (decl_type)))
> +	    warning ("removing const passing argument %d "
> +		     "to an Altivec intrinsic", n + 1);

Why are you warning?

> +  for (desc = altivec_overloaded_builtins;
> +       desc->code && desc->code != fcode; desc++)
> +    continue;

Given the size of the table, it would be nice to make this a binary
search, with a small linear scan at the end to find the beginning of
the range for fcode.

> +  { MASK_ALTIVEC, 0, "__builtin_vec_dst", ALTIVEC_BUILTIN_VEC_DST },

0 is a valid insn number; you should be using CODE_FOR_nothing.

> +      error ("unresolved overload for Altivec builtin %qs",
> +	     IDENTIFIER_POINTER (DECL_NAME (fndecl)));

%qE, fndecl ?

> #define vec_vaddcuw vec_addc

At minimum these should be function macros.  For C++ they should probably
not be macros at all.

> #define vec_step(x) __builtin_vec_step (* (__typeof__ (x) *) 0)

Using  (typeof(x)){ 0 }  would probably be friendlier to the tree
optimizers.  With this they'll see arbitrary memory being accessed
and not do dead store elimination etc.


r~