Re: [PATCH 1/5] Altera Nios II: GCC backend patches

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

On Thu, 18 Apr 2013, Chung-Lin Tang wrote:

> Index: gcc/config/nios2/nios2.h
> ===================================================================
> --- gcc/config/nios2/nios2.h	(revision 0)
> +++ gcc/config/nios2/nios2.h	(revision 409063)
> @@ -0,0 +1,686 @@
> +/* Definitions of target machine for Altera Nios II.
> +   Copyright (C) 2012 Free Software Foundation, Inc.

As noted for the libgcc patch, <year>-2013 for all new files.

> +/* Define built-in preprocessor macros.  */
> +#define TARGET_CPU_CPP_BUILTINS()                   \
> +  do                                                \
> +    {                                               \
> +      builtin_define_std ("NIOS2");                 \
> +      builtin_define_std ("nios2");                 \
> +      if (TARGET_BIG_ENDIAN)                        \
> +        builtin_define_std ("nios2_big_endian");    \
> +      else                                          \
> +        builtin_define_std ("nios2_little_endian"); \
> +    }                                               \
> +  while (0)

Do you really need the macro variants in the user namespace, or could you 
just use builtin_define with implementation-namespace variants only (like 
newer ports tend to do, e.g. AArch64 defines __aarch64__, __AARCH64EB__, 
__AARCH64EL__ as appropriate, but never any variants without the leading 
__)?

> Index: gcc/config/nios2/nios2-opts.h
> ===================================================================
> --- gcc/config/nios2/nios2-opts.h	(revision 0)
> +++ gcc/config/nios2/nios2-opts.h	(revision 409063)
> @@ -0,0 +1,70 @@
> +/* Definitions for option handling for Nios II.
> +   Copyright (C) 2013
> +   Free Software Foundation, Inc.

The use of ranges also means there's never any reason to split a copyright 
notice over multiple lines now, as it's never going to get long enough to 
need to wrap.

> Index: gcc/config/nios2/linux.h
> ===================================================================
> --- gcc/config/nios2/linux.h	(revision 0)
> +++ gcc/config/nios2/linux.h	(revision 409063)

> +#undef LIB_SPEC
> +#define LIB_SPEC "-lc \
> + %{pthread:-lpthread}"

What's wrong with using the GNU_USER_TARGET_LIB_SPEC value from 
gnu-user.h?  (If the header ordering means another header has overridden 
that definition by now, you can just redefine LIB_SPEC to 
GNU_USER_TARGET_LIB_SPEC here.)

> +#undef STARTFILE_SPEC
> +#define STARTFILE_SPEC \
> +"%{!shared: crt1.o%s} \
> + crti.o%s %{static:crtbeginT.o%s;shared|pie:crtbeginS.o%s;:crtbegin.o%s}"
> +
> +#undef ENDFILE_SPEC
> +#define ENDFILE_SPEC \
> +"%{shared|pie:crtendS.o%s;:crtend.o%s} crtn.o%s"

Again, what's wrong with the definitions from gnu-user.h?

> +#undef SYSROOT_SUFFIX_SPEC
> +#define SYSROOT_SUFFIX_SPEC \
> +  "%{EB:/EB}"

Normally, if SYSROOT_SUFFIX_SPEC is desired it isn't hardcoded like this; 
instead, ./sysroot-suffix.h is listed in tm_file and t-sysroot-suffix in 
tmake_file is used to generate sysroot-suffix.h.

> Index: gcc/config/nios2/nios2.c

> +#include <stdio.h>
> +#include "config.h"
> +#include "system.h"
> +#include "coretypes.h"

Never include system headers before "config.h", as config.h may define 
feature test macros required for system headers to work correctly and 
consistently in GCC.  If a header is included by system.h, as <stdio.h> 
is, don't include it explicitly at all.

> +/* Local prototypes.  */

Although not required, I think you could probably avoid a lot of these if 
you topologically sort functions in the file and initialize the target 
structure at the end of the file, not the start (so static prototypes 
would then generally only be needed in some cases of recursion).

> +static bool
> +nios2_fpu_compare_enabled (enum rtx_code cond, enum machine_mode mode)

Watch out for functions like this with no comment on them explaining the 
semantics of the function, its parameters and its return value.  (Where 
the function implements a standard target hook, it suffices for the 
comment to name the hook implemented rather than repeating the definition 
of the semantics of that hook.)  Many more cases in this file needing such 
comments added.

> +	      error ("switch `-mcustom-%s' is required for double precision "
> +		     "floating point", N2FPU_NAME (j));

Old-style `' quoting; use %< and %> for opening and closing quotes with 
GCC diagnostic functions.  Applies to many other error and warning 
messages in this file; I haven't listed the cases individually.

In general, consider whether an explicit location can readily be passed 
from somewhere and functions such as error_at used rather than functions 
that use the implicit global input_location.  (However, the cases for 
which this is more important - errors relating to expansion of built-in 
functions, where input_location bears little relation to the location of 
the erroneous source code construct - also tend to be cases for which a 
location may not be readily available at all, unless you have a tree 
expression and can take a location from that.)

> +  if (mode == BLKmode)
> +    {
> +      param_size = int_size_in_bytes (type);
> +      if (param_size < 0)
> +        internal_error
> +          ("Do not know how to handle large structs or variable length types");

This sounds like a case for sorry () rather than internal_error () to me - 
as I understand it, it's a matter of a known valid source-code construct 
not handled by the port, as opposed to a should-never-happen condition 
that indicates a condition you didn't think any source code could 
generate.

Note that diagnostics should start with a lowercase letter, here and 
elsewhere.

> +        internal_error
> +          ("Do not know how to handle large structs or variable length types");

Likewise.

> +## MULTILIB_OPTIONS
> +## For some targets, invoking GCC in different ways produces objects that
> +## can not be linked together.  For example, for some targets GCC produces
> +## both big and little endian code.  For these targets, you must arrange
> +## for multiple versions of libgcc.a to be compiled, one for each set of
> +## incompatible options.  When GCC invokes the linker, it arranges to link
> +## in the right version of libgcc.a, based on the command line options
> +## used.
> +## The MULTILIB_OPTIONS macro lists the set of options for which special
> +## versions of libgcc.a must be built.  Write options that are mutually
> +## incompatible side by side, separated by a slash.  Write options that may
> +## be used together separated by a space.  The build procedure will build
> +## all combinations of compatible options.
> +##
> +## For example, if you set MULTILIB_OPTIONS to m68000/m68020 msoft-float,
> +## Makefile will build special versions of libgcc.a using the following
> +## sets of options: -m68000, -m68020, -msoft-float, -m68000 -msoft-float,
> +## and -m68020 -msoft-float.

Don't include such long comments that just repeat the standard definition 
of some interface and say nothing Nios II-specific.

> +## The BUILD_BE_MULTILIB and BUILD_PG_MULTILIB variables allow the
> +## makefile user to enable/disable the generation of the precompiled
> +## big endian and profiling libraries. By default, the big endian 
> +## libraries are not created on a windows build and the profiling
> +## libraries are not created on a Solaris build. All other library 
> +## combinations are created by default.
> +
> +# Uncomment to temporarily avoid building big endian and profiling libraries during a Windows build.
> +#ifeq ($(DEV_HOST_OS), win32)
> +#BUILD_BE_MULTILIB ?= 0
> +#BUILD_PG_MULTILIB ?= 0
> +#endif

I don't know where DEV_HOST_OS is supposed to come from, but this doesn't 
appear to make much sense for FSF GCC anyway.  If you want options to have 
various possible sets of multilibs, model them on e.g. SH 
--with-multilib-list rather than having this Nios II-specific approach.

-- 
Joseph S. Myers
joseph@codesourcery.com