Re: Device specific ISA support in AVR

Ping!

> -----Original Message-----
> From: [email protected] [mailto:avr-
> [email protected]] On Behalf Of S,
> Pitchumani
> Sent: Saturday, March 15, 2014 9:20 PM
> To: Georg-Johann Lay
> Cc: [email protected]
> Subject: Re: [avr-gcc-list] Device specific ISA support in AVR
> 
> > -----Original Message-----
> > From: Georg-Johann Lay [mailto:[email protected]]
> > Sent: Thursday, March 13, 2014 7:43 PM
> > To: S, Pitchumani
> > Cc: [email protected]
> > 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  <[email protected]>
> > >
> > >      * 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  <[email protected]>
> > >
> > >      * 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  <[email protected]>
> 
>     * 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  <[email protected]>
> 
>     * gcc.target/avr/dev-specific-rmw.c: New test.