This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [Patch]Extend GAS arm_feature_set struct to provide more available bits
- From: Nicholas Clifton <nickc at redhat dot com>
- To: Terry Guo <terry dot guo at arm dot com>, binutils at sourceware dot org
- Cc: Ramana Radhakrishnan <ramana dot radhakrishnan at arm dot com>, Richard Earnshaw <Richard dot Earnshaw at arm dot com>
- Date: Tue, 10 Mar 2015 11:03:55 +0000
- Subject: Re: [Patch]Extend GAS arm_feature_set struct to provide more available bits
- Authentication-results: sourceware.org; auth=none
- References: <007001d0572b$922edf60$b68c9e20$ at arm dot com>
Hi Terry,
Currently all 32 bits in core filed of struct arm_feature_set are occupied
to represent various features.
wouldn't it have been easier to just switch to a 64-bit data type ?
Just asking...
include/opcode/ChangeLog:
2015-03-05 Terry Guo <terry.guo@arm.com>
* arm.h (arm_feature_set): Redefined to provide more available bits.
(ARM_ANY): Updated to follow above new definition.
(ARM_CPU_HAS_FEATURE): Likewise.
(ARM_CPU_IS_ANY): Likewise.
(ARM_MERGE_FEATURE_SETS): Likewise.
(ARM_CLEAR_FEATURE): Likewise.
(ARM_FEATURE): Likewise.
(ARM_FEATURE_COPY): New macro.
(ARM_FEATURE_EQUAL): Likewise.
(ARM_FEATURE_ZERO): Likewise.
(ARM_FEATURE_CORE_EQUAL): Likewise.
(ARM_FEATURE_LOW): Likewise.
gas/ChangeLog:
2015-03-05 Terry Guo <terry.guo@arm.com>
* config/tc-arm.c (no_cpu_selected): Use new macro to compare features.
(parse_psr): Likewise.
(do_t_mrs): Likewise.
(do_t_msr): Likewise.
opcodes/ChangeLog:
2015-03-05 Terry Guo <terry.guo@arm.com>
* arm-dis.c (opcode32): Updated to use new arm feature struct.
(opcode16): Likewise.
(coprocessor_opcodes): Replace bit with feature struct.
(neon_opcodes): Likewise.
(arm_opcodes): Likewise.
(thumb_opcodes): Likewise.
(thumb32_opcodes): Likewise.
(print_insn_coprocessor): Likewise.
(print_insn_arm): Likewise.
(select_arm_features): Follow new feature struct.
Approved - please apply - but...
The current macro method seems a bit messy, especially now that there
are so many features to encode and test. I would have preferred to see
a rewrite to a more extensible mechanism that could cope with an
unlimited(*) number of features without the need for future updates.
That said, if you stick with your current patch, I noticed that there a
lots of places where ARM_FEATURE_LOW is called with a 0 as its first or
second arguments. Since you are already renaming the macro why not
create a couple of new ones that eliminate the need for a redundant
argument ? Eg:
#define ARM_FEATURE_CORE_LOW(core) {{(core), 0}, 0}
#define ARM_FEATURE_COPROC(coproc) {{0, 0}, (coproc)}
It just makes the code look that little bit cleaner.
Cheers
Nick
(*) For some reasonably small value of unlimited, eg 1^32.