Re: Device specific ISA support in AVR

Am 03/27/2014 12:58 PM, schrieb S, Pitchumani:
> Ping!
>
>>>>>> 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.
>

Look good now.

Johann

(Admitting that I don't like "attribute" because there are already so much 
attributes (insn attributes, variable attributes, function attributes, ...))

But well...