This is the mail archive of the binutils@sourceware.org mailing list for the binutils 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] |
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
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |