Re: Device specific ISA support in AVR

Am 02/25/2014 01:05 PM, schrieb S, Pitchumani:
> Hi,
>
> Few AVR Xmega devices have specific instruction support than the architecture
> it belongs to. For example atxmega128b1 device has RMW instructions (XCH,LAC,
> LAS and LAT) support, but not all avrxmega6 devices have.
>
> Now, avr-gcc passes architecture name to assembler instead of device name. So,
> RMW instructions are not recognized (illegal opcode error) by assembler.
>
> To address this issue, we could add device specific ISA to device details
> in GCC. Driver can pass additional option based on specific ISA that a device
> has. Assembler can add device specific ISA to architecture ISA based on the
> option it receives.
>
> I have attached patches for avr-gcc and avr-as to address this issue.
>
> device-specific-isa-avr-gcc.patch:
> * Device specific ISA information is added to device details.
> * avr-gcc passes -mrmw option to assembler if the selected device
> has RMW instruction support.
>
> device-specific-isa-avr-as.patch:
> * Add option -mrmw.
> * Add AVR_ISA_RMW to current architecture ISA if -mrmw option specified.
>
> 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.



Johann

>
> Thanks,
> Pitchumani
>
> gcc/ChangeLog
> 2014-02-25  Pitchumani Sivanupandi  <[email protected]>
>
>      * config/avr/avr-arch.h: Add avr_additional_isa enum to have device
>      specific ISA information.
>      (avr_mcu_t): Add additional_isa field for device specific ISA.
>      * config/avr/avr-devices.c (avr_mcu_types): Update AVR_MCU macro to
>      include additional isa field.
>      * config/avr/avr-mcus.def: Update device details with corresponding
>      additional isa 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.
>
> gcc/testsuite/ChangeLog
> 2014-02-25 Pitchumani Sivanupandi  <[email protected]>
>
>      * gcc.target/avr/additional-isa.c: New test.
>
> gas/ChangeLog
> 2014-02-25 Pitchumani Sivanupandi <[email protected]>
>
>      * config/tc-avr.c: Add specified_mcu variable for selected mcu.
>      (enum options): add OPTION_RMW_ISA for -mrmw option.
>      (struct option md_longopts): Add mrmw option.
>      (md_show_usage): add -mrmw option description.
>      (md_parse_option): Update isa details if -mrmw option specified.
>      * doc/c-avr.texi: Add doc for new option -mrmw.
>
> gas/testsuite/ChangeLog
> 2014-02-25 Pitchumani Sivanupandi <[email protected]>
>
>      * gas/avr/avr.exp: Run new tests.
>      * gas/avr/rmw.d: Add test for additional ISA support.
>      * gas/avr/rmw.s: Ditto.