Please review the patches and comment.
Hi Pitchumani,
some remarks on the work:
1) It might be useful to builtin-define macros so that user code can
test for
availability of these instructions, similar to __AVR_ERRATA_SKIP__ or
__AVR_HAVE_MUL__. That way users can depend on the macro when
implementing
their inline assembler that might use RMW instructions. This macro
should then be documented with the other macros.
2) Binutils' documentation should spell out "RMW" as read-modify-
store. It's
always easier to remember an option if the resolution of some cryptic
letter combination is known.
3) There are also MCU-specific ISA-like features in avr-mcus.def:
*) ERRATA_SKIP (ISA with broken CPSE, SBRC/S, SBIC/S wrapping 32-bit
insn)
*) SHORT_SP (MCU has no SPH special function register)
Maybe these 3 ISA properties can be merged into one field with
respective
flags. That way avr-mcus.def would be easier to read and the number
of fields would reduce.
4) I'd prefer ISA in front of the feature, not as suffix, i.e.
AVR_ISA_RMW, AVR_ISA_SKIP_BUG, AVR_ISR_SP8, etc. instead of
AVR_RMW_ISA.
5) There is already a Binutils PR, cf. PR15043 (with differently
named options, though).
http://sourceware.org/PR15043
6) The GCC release notes must mention that at least Binutils version
xyz is needed.
Typically, there is some time margin until GCC might assume that
specific
features are available in binutils. This is beacuse your work is not
a pure extension but affects existing devices.
7) Don't you skip the test conditions that you want to test in the
new GCC testsuite program? I.e. you skip -mmcu=atxmega32a4u.
Hi Johann,
I have updated avr-gcc patch as per your comments. Please review.
Again some notes :-)
The GNU coding rules limit lines to 80 characters; some lines in your
changes
are longer. (There are some files like avr-mcus.def where long lines are
in
order and accepted).
Maybe the easiest way is to rename lengthy component name
"dev_specific_feature" to "isa" which is clear enough, IMO.
The test case is still not in order because of
+/* { dg-options "-mmcu=atxmega128b1" } */
which collides with -mmcu= when the tests are run for a different
target.
But there's no need for -mmcu= at all, we have __AVR_ISA_RMW__ so you
can
factor out the asms by means of #ifdef __AVR_ISA_RMW__ :-)
Johann
gcc/ChangeLog
2014-03-12 Pitchumani Sivanupandi <address@hidden>
* config/avr/avr-arch.h (avr_mcu_t): Add dev_specific_feature
field
to have device specific ISA/ feature information. Remove short_sp
and errata_skip fields.
Add avr_device_specific_features enum to have device specific
info.
* config/avr/avr-c.c: use dev_specific_feature to check
errata_skip.
Function names are missing.
Add __AVR_ISA_RMW__ builtin macro if RMW ISA available.
* config/avr/avr-devices.c (avr_mcu_types): Update AVR_MCU macro
for
updated device specific info.
* config/avr/avr-mcus.def: Merge device specific details to
dev_specific_feature field.
* config/avr/avr.c (avr_2word_insn_p): use dev_specific_feature
field
to check errata_skip.
* config/avr/avr.h: same for short sp info.
What objects / macros are affected?
* config/avr/driver-avr.c (avr_device_to_as): Pass -mrmw option
to
assembler if RMW isa supported by current device.
* config/avr/genmultilib.awk: Update as device info structure
changed.
* doc/invoke.texi: Add info for __AVR_ISA_RMW__ builtin macro
gcc/testsuite/ChangeLog
2014-03-12 Pitchumani Sivanupandi <address@hidden>
* gcc.target/avr/ dev-specific-rmw.c: New test.
Superfluous blank in the file name.