Re: [avr-gcc-list] Device specific ISA support in AVR

[Georg-Johann Lay]

Am 03/12/2014 06:59 PM, schrieb S, Pitchumani:

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