Re: [patch] convert some call ABI macros to hooks, apply to sh, add bitfield swapper

[Alexandre Oliva <aoliva at redhat dot com>]

I'm going through the SH-specific changes only.  I have only cosmetic
changes to request, assuming the changes from macro to functions were
only mechanical, such that I don't have to check that the logic is
unchanged.  Is this the case?  If not, could you please highlight the
changes (maybe even submit them separately, if it's not too
difficult)?  Thanks,

On Aug 13, 2003, DJ Delorie <dj@redhat.com> wrote:

> !    Copyright (C) 1993, 1994, 1995, 1997, 1997, 1998, 1999, 2000, 2001, 2002, 2003,
>      2003 Free Software Foundation, Inc.

Oops, 2003 is now duplicated in sh.c

> !   if (TARGET_SH5 || (! TARGET_SH2E && ! TARGET_SH4) || TARGET_HITACHI
> !       || sh_cfun_attr_renesas_p ())

Would you mind moving TARGET_HITACHI down to the same line as
sh_cfun_attr_renesas_p() every time they occur together, like you did
in sh_expand_prologue()?

> +   if (!type)
> +     return 1;
> +   return ! sh_attr_renesas_p (type);

This is not self-consistent in terms of whitespace after `!'.  I don't
recall which is right, but one of these is probably wrong.

> + sh_function_arg (ca, mode, type, named)

I'd appreciate if new functions were added with ISO C prototypes.

> +       && PASS_IN_REG_P ((*ca), (mode), (type))

It is safe to remove the extra parentheses now, since we're no longer
dealing with incoming macro arguments.

> +       && ((named) || !(TARGET_HITACHI || (*ca).renesas_abi)))

What's this with (*ca).something?  Any reason to not use ca->something
instead?

> + 	  rtx r2 = gen_rtx_EXPR_LIST(VOIDmode,
                                    ^ blank
> + 	  return gen_rtx_PARALLEL(SCmode, gen_rtvec(2, r1, r2));
                                 ^ blank
> +   

> +   /*  dump_ca (ca, __LINE__, "in sh_function_arg_advance");*/

?!?

> +      tree TYPE_ = ((*ca).byref && (type)

Please rename this variable, it had this ugly name only to avoid
hiding names in contexts enclosing the former macro invocation.

> +      enum machine_mode MODE_ = ((*ca).byref && (type)

Likewise.

> + 	      || ((TARGET_HITACHI || sh_attr_renesas_p(fndecl))
                                                      ^ blank

> + static void
> + sh_setup_incoming_varargs (ca, mode, type, pretend_arg_size, second_time)
> +      CUMULATIVE_ARGS *ca;
> +      enum machine_mode mode;
> +      tree type;
> +      int *pretend_arg_size;
> +      int second_time;

None of the arguments are used here.  Please add ATTRIBUTE_UNUSED
where appropriate, in all new functions.  -Wall (enabled by default?)
should let you know where to add them.

> + /* True if __attribute__((renesas)) and not -mrenesas.  */

Err...  This doesn't sound right.  This seems to be true if -mrenesas
(in both functions).  Also, any reason for these functions to not
return a bool?

> !    Copyright (C) 1993, 1994, 1995, 1996, 1997, 1998, 1999, 2000, 2001, 2002, 2003,
>      2003 Free Software Foundation, Inc.

sh.h has two occurrences of 2003 now.

> +   /* This is set to non-zero when the call in question must use the Renesas ABI,
> +      even without the -mrenesas option.  */
> +     int renesas_abi;

Could I perhaps convince you to arrange for renesas_abi to be passed
as a call cookie to non-SHcompact non-SHmedia call patterns?  We're
going to be able to tell which ABI a function uses by just looking at
the call insn, and there's no other way to do it, especially in case
of indirect calls (or calls made indirect after the callee symbol is
placed in the constant pool).

> !       = ((TARGET_HITACHI || (CUM).renesas_abi) && FNTYPE	\

Missing parentheses around FNTYPE

> ! 	 && aggregate_value_p (TREE_TYPE (FNTYPE), FNDECL));	\

and around FNDECL

> ! 	 && aggregate_value_p (TREE_TYPE (FNTYPE), FNDECL));	\

and FNDECL again

> ! 	&& (! (TARGET_HITACHI || (CUM).renesas_abi) \
> ! 	    || ! (AGGREGATE_TYPE_P (TYPE) \
> ! 		  || (!TARGET_FPU_ANY && ((MODE) == DFmode \
> ! 				       || (MODE) == TFmode)))))) \

Are DFmode values passed in FP regs if single-only?  I wouldn't think
so.  A comment justifying the logic above would be welcome.  You may
want to key on MODE_CLASS and mode size.

-- 
Alexandre Oliva   Enjoy Guarana', see http://www.ic.unicamp.br/~oliva/
Red Hat GCC Developer                 aoliva@{redhat.com, gcc.gnu.org}
CS PhD student at IC-Unicamp        oliva@{lsd.ic.unicamp.br, gnu.org}
Free Software Evangelist                Professional serial bug killer