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: C6X port 10/11: The port


On Tue, 10 May 2011, Bernd Schmidt wrote:

> 	* doc/invoke.texi (C6X Options): New section.
> 	* doc/md.texi (TI C6X family): New section.
> 	* config.gcc: Handle tic6x, in particular tic6x-*-elf and
> 	tic6x-*-uclinux.
> 	config/c6x/uclinux-elf.h: New file.
> 	config/c6x/predicates.md: New file.

[...]

The new files seem to be listed in random order.  If you don't have a 
better order, I advise alphabetical order - *not* the random order "svn 
diff" uses.  (The ChangeLog entries should also all have a leading "* ".)

General comment: there are lots of new files here that are built for the 
target, and if possible it's preferable for such sources to be under 
libgcc/config/ with associated build rules also located there not in the 
gcc/ directory.

> +tic6x-*-uclinux)
> +	tm_file="elfos.h ${tm_file} dbxelf.h c6x/elf.h tm-dwarf2.h c6x/uclinux-elf.h"

All targets based on the Linux kernel (with or without MMU) should now be 
using gnu-user.h and linux.h (and then overriding anything inappropriate 
from those headers).  There's a legacy issue that alpha*-*-linux* 
arm*-*-uclinux* powerpc-*-linux* powerpc64-*-linux* don't use those 
headers, but new Linux targets not using them should not be added.

> +	tm_file="$tm_file glibc-stdint.h"
> +       	tmake_file="c6x/t-c6x c6x/t-c6x-elf c6x/t-c6x-uclinux c6x/t-opts"

Indentation seems odd here.

> +	tmake_file="$tmake_file c6x/t-c6x-softfp soft-fp/t-softfp"
> +	tm_file="$tm_file ./sysroot-suffix.h"
> +	tmake_file="$tmake_file t-sysroot-suffix"
> +	tmake_file="$tmake_file t-slibgcc-elf-ver"
> +       	use_collect2=no

And again.

You're using t-slibgcc-elf-ver, so presumably building shared libgcc with 
symbol versioning - but I don't see any C6X-specific version map to assign 
symbol versions to C6X-specific symbols (both __c6xabi_*, and the __gnu_* 
renamings of some GCC symbols to put them in a proper vendor namespace).  
Even if some functions have special ABIs preventing them from being called 
through the PLT, I'd expect most of the functions to be exported from 
shared libgcc.

> +	tic6x-*-*)
> +		supported_defaults="arch"
> +
> +		case ${with_arch} in
> +		"" | c64x+ | c674x)
> +			# OK
> +			;;
> +		*)
> +			echo "Unknown arch used in --with-arch=$with_arch." 1>&2
> +			exit 1
> +			;;

That list is inconsistent with the full set of six CPU variants supported 
by the port; I'd think all should be handled for --with-arch, even if the 
port isn't particularly well tuned for the older variants or some features 
don't work for them.

> +	  /* Treat any failure as the end of unwinding, to cope more
> +	     gracefully with missing EH information.  Mixed EH and
> +	     non-EH within one object will usually result in failure,
> +	     because the .ARM.exidx tables do not indicate the end
> +	     of the code to which they apply; but mixed EH and non-EH

Bogus ARM reference in a comment (and, Paul implemented linker support for 
closing address ranges for both ARM and C6X).

> +/* Common implementation for ARM ABI defined personality routines.

Another bogus ARM reference.

> Index: config/c6x/t-opts
> ===================================================================
> --- /dev/null
> +++ config/c6x/t-opts
> @@ -0,0 +1,4 @@
> +$(srcdir)/config/c6x/c6x-tables.opt: $(srcdir)/config/c6x/genopt.sh \
> +  $(srcdir)/config/c6x/c6x-isas.def
> +	$(SHELL) $(srcdir)/config/c6x/genopt.sh $(srcdir)/config/c6x > \
> +		$(srcdir)/config/c6x/c6x-tables.opt

I don't think it makes sense for this to be separate from t-c6x.  m68k has 
t-opts because t-m68k *isn't* used for all m68k targets (only for classic 
m68k as opposed to ColdFire); that issue doesn't apply here.  (I'm not 
convinced it makes sense to separate t-c6x and t-c6x-elf either.)

> +#ifndef UNWIND_ARM_H
> +#define UNWIND_ARM_H

Is something elsewhere testing this particular spelling?  If not, ARM 
should not be referenced here.

> +#endif /* defined UNWIND_ARM_H */

Likewise.

> +#undef CC1_SPEC
> +#define CC1_SPEC "%{G*}"

You're not using g.opt, so the -G option doesn't exist for this target, so 
this spec is suspicious.

> +/* Make __c6xabi_AEABI_NAME an alias for __GCC_NAME.  */
> +#define RENAME_LIBRARY(GCC_NAME, AEABI_NAME)			\
> +  __asm__ (".globl\t__c6xabi_" #AEABI_NAME "\n"		\
> +	   ".set\t__c6xabi_" #AEABI_NAME			\
> +	   ", __" #GCC_NAME "\n");
> +
> +/* Rename helper functions to the names specified in the C6000 ELF ABI.  */
> +#ifdef L_divsi3
> +#define DECLARE_LIBRARY_RENAMES RENAME_LIBRARY (divsi3, divi)
> +#endif

All this code now needs to go in a header in libgcc/config/, listed in 
libgcc_tm_file.

> +$(srcdir)/config/c6x/c6x-sched.md: $(srcdir)/config/c6x/gensched.sh \
> +  $(srcdir)/config/c6x/c6x-sched.md.in
> +	$(SHELL) $(srcdir)/config/c6x/gensched.sh \
> +	$(srcdir)/config/c6x/c6x-sched.md.in > $@
> +
> +$(srcdir)/config/c6x/c6x-mult.md: $(srcdir)/config/c6x/genmult.sh \
> +  $(srcdir)/config/c6x/c6x-mult.md.in
> +	$(SHELL) $(srcdir)/config/c6x/genmult.sh \
> +	$(srcdir)/config/c6x/c6x-mult.md.in > $@

Generated files in the source tree should be accompanied by 
timestamp-updating rules on contrib/gcc_update (this also applies to the 
file generated by t-opts).

> +msdata=
> +Target RejectNegative Enum(c6x_sdata) Joined Var(c6x_sdata_mode) Init(C6X_SDATA_DEFAULT)
> +Select method for sdata handling
> +
> +Enum
> +Name(c6x_sdata) Type(enum c6x_sdata)
> +

It's desirable for --target-help to show the list of valid values, either 
in the help for the individual option (as for various Enum options in 
common.opt, using the TAB-separated form of help text) or by adding a help 
text to the Enum definition.

> Index: config/c6x/c6x.c

> +#include "tm.h"

> +#include "toplev.h"

Most targets no longer need to include toplev.h, does this one actually 
use any functions from it?

> +#include "c6x.h"

Redundant with the above include of tm.h.

> +#include "c6x-protos.h"

Would normally be included via tm_p.h.

> +#if 0 /* FIXME: Reenable when TI's tools are fixed.  */
> +  /* ??? Ideally we'd check flag_short_wchar somehow.  */
> +  asm_fprintf (asm_out_file, "\t.c6xabi_attribute Tag_ABI_wchar_t, %d\n", 2);
> +#endif

ARM has arm_output_c_attributes in arm-c.c.

> Index: config/c6x/c6x-sched.md
> ===================================================================
> --- /dev/null
> +++ config/c6x/c6x-sched.md
> @@ -0,0 +1,838 @@
> +;; -*- buffer-read-only: t -*-
> +;; Generated automatically from c6x-sched.md.in by gensched.sh
> +

Should have copyright/license notice (probably copied from the input 
file).

> +/* Define this to nonzero if the nominal address of the stack frame
> +   is at the high-address end of the local variables;
> +   that is, each additional local variable allocated
> +   goes at a more negative offset in the frame.  */
> +#define FRAME_GROWS_DOWNWARD 1
> +
> +/* Define this if pushing a word on the stack
> +   makes the stack pointer a smaller address.  */
> +#define STACK_GROWS_DOWNWARD

These look like generic comments (which should be avoided in target 
headers) instead of saying anything C6X-specific.

> +#define FUNCTION_PROFILER(file, labelno) \
> +  fatal_error("Profiling is not yet implemented for this architecture.")

Diagnostics should not start with a capital letter or end with "." (and 
note missing space before "(" in function call).

> +/* Definitions for register eliminations.
> +
> +   This is an array of structures.  Each structure initializes one pair
> +   of eliminable registers.  The "from" register number is given first,
> +   followed by "to".  Eliminations of the same "from" register are listed
> +   in order of preference.  */

Generic.

> +/* `LEGITIMATE_PIC_OPERAND_P (X)'
> +     A C expression that is nonzero if X is a legitimate immediate
> +     operand on the target machine when generating position independent
> +     code.  You can assume that X satisfies `CONSTANT_P', so you need
> +     not check this.  You can also assume FLAG_PIC is true, so you need
> +     not check it either.  You need not define this macro if all
> +     constants (including `SYMBOL_REF') can be immediate operands when
> +     generating position independent code. */

Generic.

> +/* A C expression to modify the code described by the conditional if
> +   information CE_INFO with the new PATTERN in INSN.  If PATTERN is a null
> +   pointer after the IFCVT_MODIFY_INSN macro executes, it is assumed that that
> +   insn cannot be converted to be executed conditionally.

Generic.

> +
> +   The ADDA insns used for accessing small data aren't predicable.  */

This seems to be the only bit of this comment that's C6X-specific.

> +/* Define this macro if it is as good or better to call a constant
> +   function address than to call an address kept in a register.  */

Generic.

> Index: config/c6x/libunwind.S
> ===================================================================
> --- /dev/null
> +++ config/c6x/libunwind.S
> @@ -0,0 +1,133 @@
> +.text

Appears to need standard libgcc copyright/license notice.

> Index: config/c6x/crti.s
> ===================================================================
> --- /dev/null
> +++ config/c6x/crti.s
> @@ -0,0 +1,17 @@
> +/*
> + * This file just supplies function prologues for the .init and .fini
> + * sections.  It is linked in before crtbegin.o.
> + */

Likewise.

> Index: config/c6x/crtn.s
> ===================================================================
> --- /dev/null
> +++ config/c6x/crtn.s
> @@ -0,0 +1,19 @@
> +/*
> + * This file supplies function epilogues for the .init and .fini sections.
> + * It is linked in after all other files.
> + */

Likewise.

-- 
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]