This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] New port for TILEPro and TILE-Gx 3/7: gcc port


All comments here refer to the first instance of an issue; many issues are 
shared between the two ports but are only mentioned once here.

On Sat, 15 Oct 2011, Walter Lee wrote:

> +#undef SYSROOT_SUFFIX_SPEC
> +#define SYSROOT_SUFFIX_SPEC "%{mbme:/usr/lib/bme;mnewlib:/usr/lib/newlib}"
> +
> +#undef SYSROOT_HEADERS_SUFFIX_SPEC
> +#define SYSROOT_HEADERS_SUFFIX_SPEC SYSROOT_SUFFIX_SPEC

These specs seem rather unusual.  Typically such specs would be generated 
depending on what multilibs were being built (see t-sysroot-suffix), and 
since a subdirectory sysroot itself contains directories such as usr/lib 
and usr/include, you would not expect a /usr/lib path within those specs.

It appears from the documentation that these are options for alternative C 
libraries.  There is an existing infrastructure for options for 
alternative C libraries used with the Linux kernel, with the -mbionic, 
-mglibc and -muclibc options all of which use Negative so that only the 
last one is considered for specs processing, all of which set a single 
variable and which affect the predefined macros, settings of 
TARGET_C99_FUNCTIONS and TARGET_HAS_SINCOS, etc.; you should integrate 
these options properly with that infrastructure.

But I don't see any sign of multilib settings to cause GCC's libraries to 
be built for these variants - although libstdc++, for example, definitely 
depends on libc in various ways, so you certainly can't expect to be able 
to use a different libc without having a separate multilib for it.

(I am presuming that these are for libraries used in userspace with the 
Linux kernel or as part of building the Linux kernel; if they aren't used 
with Linux at all then a separate -elf or -eabi target would be the normal 
approach.)

> +#undef ASM_SPEC
> +#define ASM_SPEC "%{v:-V} %{Qy:} %{!Qn:-Qy} %{n} %{T} %{Ym,*} %{Yd,*} \
> + %{Wa,*:%*} %{m32:--32} %{m64:--64}"

All of this apart from %{m32:--32} %{m64:--64} is obsolete.  See a series 
of my patches that cleaned up obsolete pieces in specs.  
<http://gcc.gnu.org/ml/gcc-patches/2010-12/msg00189.html> for -Wa,; 
<http://gcc.gnu.org/ml/gcc-patches/2010-12/msg00192.html> for -n and -T; 
<http://gcc.gnu.org/ml/gcc-patches/2010-12/msg00195.html> for -v; 
<http://gcc.gnu.org/ml/gcc-patches/2011-01/msg00893.html> for -Yd,; 
<http://gcc.gnu.org/ml/gcc-patches/2011-01/msg00913.html> for -Ym,; 
<http://gcc.gnu.org/ml/gcc-patches/2011-01/msg00914.html> for -Qn and -Qy.

> +      %{!dynamic-linker:-dynamic-linker \

%{!dynamic-linker:...} doesn't do what you think and is also obsolete; 
again see my patches 
(<http://gcc.gnu.org/ml/gcc-patches/2010-12/msg00194.html> in this case).

> +#undef  STARTFILE_SPEC
> +#define STARTFILE_SPEC "\
> +  %{!shared: %{pg|p|profile:gcrt1.o%s;pie:Scrt1.o%s;:crt1.o%s}} \
> +  crti.o%s \
> +  %{static:crtbeginT.o%s;shared|pie:crtbeginS.o%s;:crtbegin.o%s} \
> +  %{ffeedback-emit*:crtfbi.o%s}"

There are no -ffeedback-emit* or -ffeedback-generate options in FSF GCC so 
there should be no specs referencing them.

Without that, this looks identical to GNU_USER_TARGET_STARTFILE_SPEC in 
gnu-user.h so you shouldn't need to override that at all (but if you just 
need to add to it, then appeand to GNU_USER_TARGET_STARTFILE_SPEC rather 
than replicating its contents).

> +#undef  LIB_SPEC
> +#define LIB_SPEC \
> +  "%{ffeedback-generate*|ffeedback-emit*:-lgcov -linstr} \
> +   %{pthread:-lpthread} \
> +   %{shared:-lc} \
> +   %{!shared:%{mieee-fp:-lieee} %{profile:-lc_p}%{!profile:-lc}}"

You don't have a -mieee-fp option in your .opt file, so shouldn't have a 
spec for it.

> +#undef MCOUNT_NAME
> +#define MCOUNT_NAME "mcount"

For a new target it seems much better to define your ABI to use a name in 
the reserved namespace for this - that is, starting with two underscores.

> +/* For __clear_cache in libgcc2.c.  */
> +#ifdef IN_LIBGCC2
> +
> +#include <arch/icache.h>

Where does this header come from?  Linux kernel, glibc, somewhere else?  
In general you want to condition header includes on inhibit_libc to 
facilitate bootstrapping (including building a partial static libgcc) 
before the libc headers are installed, since configuring glibc to install 
its headers requires a working compiler to run configure tests.

> diff -r -u -p -N /home/packages/gcc-4.7.0-179959/gcc/config/tilegx/mul_tables.c ./gcc/config/tilegx/mul_tables.c
> --- /home/packages/gcc-4.7.0-179959/gcc/config/tilegx/mul_tables.c	1969-12-31 19:00:00.000000000 -0500
> +++ ./gcc/config/tilegx/mul_tables.c	2011-10-14 18:14:11.524757000 -0400
> @@ -0,0 +1,19853 @@

Are you really sure that this 19853-line file is source code - "the 
preferred form of the work for making modifications to it"?  How was it 
written?  If it was generated by a program, and modifying that program 
would be the preferred way of changing this file, then the genuine source 
code for that program needs to be included as well.  The genuine source 
code may not be used at build time - the ARM port has various code 
generated by O'Caml generators, for example - but at least it needs to be 
documented how someone, using the shipped source code and free software 
implementations of any relevant languages, can regenerate the generated 
file exactly.  And if the generation is easy and doesn't require unusual 
tools, the rules for doing so should go in the makefiles (if for whatever 
reason a generated file still needs shipping in the source tree, also 
update contrib/gcc_update to avoid accidental regeneration).

> +tilegx-c.o: $(srcdir)/config/tilegx/tilegx-c.c \
> +    $(CONFIG_H) $(SYSTEM_H) coretypes.h $(MACHMODE_H) \
> +    $(TM_H) $(TM_P_H) $(CPPLIB_H) $(TREE_H) $(C_COMMON_H)
> +	$(CC) -c $(ALL_CFLAGS) $(ALL_CPPFLAGS) $(INCLUDES) \
> +	  $(srcdir)/config/tilegx/tilegx-c.c
> +
> +mul_tables.o: $(srcdir)/config/tilegx/mul_tables.c \
> +    $(CONFIG_H) $(SYSTEM_H) coretypes.h $(EXPR_H) $(OPTABS_H) \
> +    $(srcdir)/config/tilegx/tilegx-multiply.h
> +	$(CC) -c $(ALL_CFLAGS) $(ALL_CPPFLAGS) $(INCLUDES) \
> +	  $(srcdir)/config/tilegx/mul_tables.c

When the compiler is built as C++, all source of the compiler should 
normally also be built as C++.  That is, you should use $(COMPILER), not 
$(CC).  You should also verify that if you build the compiler using a 
current trunk compiler as your starting point, it passes a 
--enable-werror-always build as both C and C++; if you have only tested 
with cross compiler builds rather than native bootstrap, those don't use 
-Werror by default, hence the need to test with --enable-werror-always and 
to use a recent trunk compiler as the starting point.

> +#ifndef _TILEGX_MULTIPLY_H
> +#define _TILEGX_MULTIPLY_H

Header include guards in host-side code like this should not be in the 
reserved namespace; use e.g. GCC_TILEGX_MULTIPLY_H instead.

> +#ifndef __TILEGX_PROTOS_H__
> +#define __TILEGX_PROTOS_H__

Likewise.

> +#include "errors.h"

errors.h is the header for cut-down diagnostic functions used in generator 
programs.  Normal compiler code should use diagnostic-core.h instead.

> +/* Implement TARGET_OPTION_OVERRIDE.  */
> +static void
> +tilegx_option_override (void)
> +{
> +  static const struct cpu_table {
> +    const char *const name;
> +    const enum processor_type processor;
> +  } cpu_table[] = {
> +    { "tilegx", PROCESSOR_TILEGX },
> +    { NULL, PROCESSOR_MAX }
> +  };
> +  const char *cpu_string;
> +  int i;
> +
> +  /* Default to TILE-GX.  */
> +  cpu_string = tilegx_cpu_string;
> +  if (cpu_string == NULL)
> +    cpu_string = "tilegx";
> +
> +  for (i = 0; cpu_table[i].name; i++)
> +    if (!strcmp (cpu_string, cpu_table[i].name))
> +      {
> +	tilegx_cpu = cpu_table[i].processor;
> +	break;
> +      }
> +
> +  if (!cpu_table[i].name)
> +    error ("bad value %s for -mcpu switch", cpu_string);

Is there a reason all this needs to use this hook rather than the generic 
.opt file Enum facility for options with enumerated arguments?

> +/* Return the simd variant of the constant NUM of mode MODE, by
> +   replicating it to fill an interger of mode DImode.  NUM is first
> +   truncated to fit in MODE.  */
> +rtx
> +tilegx_simd_int (rtx num, enum machine_mode mode)
> +{
> +  HOST_WIDE_INT n = 0;
> +
> +  if (CONST_INT_P (num))
> +    {
> +      n = INTVAL (num);
> +    }
> +  else
> +    {
> +      error ("Immediate operand to SIMD op not a constant.");
> +    }

Error messages should not start with a capital letter or end with '.'.

Is this a message about a bad argument to a built-in function, or 
something like that?  If not (if no input should ever generate it), then 
I'd think it should be an internal error, not a normal one.  But if it 
does come from built-in expansion then this point is probably too late to 
get a good diagnostic location; ideally you'd use error_at with a location 
derived from the relevant expression, and generate errors early enough for 
that to be possible.

> +    if (!(*insn_op->predicate) (op[opnum], insn_op->mode))
> +      {
> +	/* We still failed to meet the predicate even after moving
> +	   into a register. Assume we needed an immediate.  */
> +	error ("operand must be an immediate of the right size");

Here's another case where explicit locations would be better.  You may at 
least be able to check EXPR_HAS_LOCATION (exp) and use the expression's 
location if so.

> +	  output_operand_lossage ("Invalid operand for 'r' specifier.");

Again, diagnostics should not start with an uppercase letter or end with a 
period.

> +@smallexample
> +
> +unsigned long long __insn_op (...)
> +
> +@end smallexample
> +
> +Where ``op'' is the name of the instruction.  Refer to the ISA manual
> +for the complete list of instructions.

Should use @var{op} in both the example and in the main text in place of 
``op''.

> +tilegx-*-linux*)
> +	tm_file="elfos.h gnu-user.h linux.h glibc-stdint.h tilegx/linux.h ${tm_file}"
> +        tmake_file="${tmake_file} tilegx/t-tilegx"
> +	extra_headers="tilegx-builtins.h tilegx-multiply.h"
> +	extra_objs="mul_tables.o"
> +	c_target_objs="tilegx-c.o"
> +	cxx_target_objs="tilegx-c.o"
> +	;;
> +tilepro-*-linux*)
> +	tm_file="elfos.h gnu-user.h linux.h glibc-stdint.h tilepro/linux.h ${tm_file}"
> +        tmake_file="${tmake_file} tilepro/t-tilepro"
> +	extra_headers="tilepro-builtins.h tilepro-multiply.h"

extra_headers is for headers for the *target*, that should be installed 
with the compiler and should have the runtime library license exception, 
not for headers that are part of the compiler on the *host*.

-- 
Joseph S. Myers
joseph@codesourcery.com


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]