[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [avr-gcc-list] Device specific ISA support in AVR
From: |
S, Pitchumani |
Subject: |
Re: [avr-gcc-list] Device specific ISA support in AVR |
Date: |
Sat, 15 Mar 2014 15:50:27 +0000 |
> -----Original Message-----
> From: Georg-Johann Lay [mailto:address@hidden
> Sent: Thursday, March 13, 2014 7:43 PM
> To: S, Pitchumani
> Cc: address@hidden
> Subject: Re: [avr-gcc-list] Device specific ISA support in AVR
>
> 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.
Hi Johann,
Thanks for the review :)
I have updated the patch and ChangeLog.
Regards,
Pitchumani
gcc/ChangeLog
2014-03-12 Pitchumani Sivanupandi <address@hidden>
* config/avr/avr-arch.h (avr_mcu_t): Add dev_attribute 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 (avr_cpu_cpp_builtins): use dev_attribute to check
errata_skip. 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_attribute field.
* config/avr/avr.c (avr_2word_insn_p): use dev_attribute field to check
errata_skip.
* config/avr/avr.h (AVR_HAVE_8BIT_SP): same for short sp info.
* 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.
dev-specific-feature-avr-gcc.patch
Description: dev-specific-feature-avr-gcc.patch