Re: [10/11] TI C6X binutils port: gas/

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

On Wed, 24 Mar 2010, DJ Delorie wrote:

> +#ifdef TC_IA64
> +#define PREDICATE_START '('
> +#define PREDICATE_END ')'
> +#endif
> +#ifdef TC_TIC6X
> +#define PREDICATE_START '['
> +#define PREDICATE_END ']'
> +#endif
> 
> Should we consider moving these to the tc-*.h files?  We'd need a
> slightly more global-aware name for them, though.

I've changed this to TC_PREDICATE_START_CHAR and TC_PREDICATE_END_CHAR.

> +#ifndef TC_TIC6X
> +	      /* For TI C6X, we keep these spaces as they may separate
> +		 functional unit specifiers from operands.  */
>  	      if (scrub_m68k_mri)
> +#endif

Although this is a conditional for a single target and app.c is full of 
those, I've changed it to TC_KEEP_OPERAND_SPACES.

> +#define TRUNC(X)	((valueT) (X) & 0xffffffff)
> +#define SEXT(X)		((TRUNC (X) ^ 0x80000000) - 0x80000000)
> 
> These constants need 'U' suffixes (and perhaps UL but I don't think so
> for the hosts we care about) to avoid compile-time warnings.  Please
> make sure you haven't disabled -Wall/-Werror during your testing.

My testing has -Wall -Werror on both 32-bit and 64-bit hosts and I have 
not seen any problems with these definitions (which are used in tc-m68k.c 
without 'U' suffixes), but I've added the suffixes.

> Also, same comment about commenting abort()s - make sure none of them
> can be triggered by user input, comment each with why they might happen,
> etc.

As with opcodes, I think many are self-explanatory and do not need 
comments, but I've added comments where I think they are useful.

> +	this_opc_fu_ok = FALSE;;
> 
> Extra semicolons.

Fixed.

> +  if (num_matching_opcodes == 0)
> +    abort ();
> 
> Do you really want an abort() here?  There are no cases where a group of
> same-named opcodes don't cover 100% of the cases the syntax allows for?

This is before operands have been parsed.  It is asserting that the only 
ways the previous loop looking for opcodes matching the architecture and 
functional unit specifier could have failed, given that the opcode name is 
valid at all, are the cases that by that point would have resulted in one 
of the specific errors just above, after which the function returns.

> +    case BFD_RELOC_NONE:
> +      /* Force output to the object file.  */
> +      fixP->fx_done = 0;
> +      break;
> 
> What are you expecting in the object file?

An R_C6000_NONE relocation, indicating that the object file depends on a 
particular symbol.  See how such relocations are generated for ARM EABI 
personality routines.  The C6000 EABI doesn't mention these relocations 
for personality routines, or the names of those routines, but that appears 
to be an omission in the ABI document; the TI tools do generate such 
relocations.  These relocations may not yet be generated by the GNU 
assembler but will be once EH support is implemented.

> +	  newval &= 0xff80007f;
> 
> You need a U suffix here, and a few other similar places.
> 
> +	  newval &= 0xff8000ff;
> +	  newval |= ((value >> 1) & 0x7fff) << 8;
> 
> There are nine of these.  You might consider making this construct a
> macro, if the shift patterns are predictable/computable enough.

I've changed these to use a macro MODIFY_VALUE.  This in turn showed up 
that one of the masks was actually wrong, so fixed an assembler bug.

> 
> +	  /* Handle a constant found to be such at fix time the same
> +	     as one found to be such on the first assembly pass.  */
> +	  if (fixP->tc_fix_data.fix_adda && fixP->fx_done)
> +	    value <<= 1;
> 
> I read this a bunch of times, I think it means "if we saw a constant the
> first time around, and did something to it, and we see the same constant
> this time around, do the same thing to it."  Yes?  If so, I suggest two
> commas (or a rewrite ;) :

I've changed the constant in a way that hopefully makes it clearer.  The 
relevant testcases are in insns-c674x-reloc.s, where ADDA instructions are 
used with operands that are the difference of two labels, and how that 
difference is handled should not depend on whether the labels are before 
or after the instruction.

> Perhaps some whitespace here?

Added.

> +The following values of @var{arch} are accepted: @code{c62x},
> +@code{c64x}, @code{c64x+}, @code{c67x}, @code{c67x+}, @code{c674x}.
> 
> We allow target triplets like tic6x-elf, should we allow arches like
> -march=tic64x ?

I don't think so.  The -march values should represent what is usual within 
the C6X context, while target triplets are meant to be globally 
meaningful.  Options for individual cores (expanding the "x" part) might 
in principle make sense, but in practice be problematic to maintain.

> +Enable or disable the optional C64x+ atomic operation instructions.
> 
> You should describe the interactions between -march and -matomic, and
> the default if no -m[no-]atomic is given.

Explained.

> Your gas port doesn't yet support the compact instructions.  Is this
> planned for the future, or perhaps as some form of relaxation?

I hope to add that support in future (definitely for objdump, hopefully 
for gas as well).  It's quite a complicated whole-function optimization 
problem; the separation of information between individual instructions and 
the fetch packet header can mean that optimal compacting requires 
reordering instructions within an execute packet, and alignment 
requirements can mean that optimal compacting early in the function 
depends on code much later in the function.  It can however definitely be 
done in linear time, at least if you simplify questions of how compacting 
affects the ranges available for branch offsets.  (Alignment complications 
can arise even without compact instructions; see "Improvements to code 
alignment support in the assembler." on my list of future features; I 
expect to do that one quite soon as it can affect correctness.)

-- 
Joseph S. Myers
joseph@codesourcery.com

Attachment: c6x-10a-gas.bz2
Description: Binary data