On 29/06/16 09:43, James Greenhalgh wrote:
> On Mon, Jun 27, 2016 at 03:58:00PM +0100, Jiong Wang wrote:
>> On 07/06/16 09:46, Jiong Wang wrote:
>>> 2016-06-07 Matthew Wahab<matthew.wa...@arm.com>
>>> Jiong Wang<jiong.w...@arm.com>
>>>
>>> * config/aarch64/aarch64-arches.def: Add "armv8.2-a".
>>> * config/aarch64/aarch64.h (AARCH64_FL_V8_2): New.
>>> (AARCH64_FL_F16): New.
>>> (AARCH64_FL_FOR_ARCH8_2): New.
>>> (AARCH64_ISA_8_2): New.
>>> (AARCH64_ISA_F16): New.
>>> (TARGET_FP_F16INST): New.
>>> (TARGET_SIMD_F16INST): New.
>>> * config/aarch64/aarch64-option-extensions.def: New entry
>>> for "fp16".
>>> * config/aarch64/aarch64-c.c (arch64_update_cpp_builtins):
>>> Conditionally define
>>> __ARM_FEATURE_FP16_SCALAR_ARITHMETIC and
>>> __ARM_FEATURE_FP16_VECTOR_ARITHMETIC.
>>> * doc/invoke.texi (AArch64 Options): Document "armv8.2-a"
>>> and "fp16".
>>>
>>
>> This is a updated version of this patch, the updates are:
>>
>> * When enabling "fp16" also enables "fp".
>> * When disabling "fp" also disables "fp16".
>> * When disabling "fp16" only disables "fp16".
>>
>> OK for trunk?
>
> Hi Jiong,
>
> Some very minor comments below.
>
>> diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
>> index b15c23f0..d715da7 100644
>> --- a/gcc/config/aarch64/aarch64.h
>> +++ b/gcc/config/aarch64/aarch64.h
>> @@ -135,6 +135,8 @@ extern unsigned aarch64_architecture_version;
>> /* ARMv8.1 architecture extensions. */
>> #define AARCH64_FL_LSE (1 << 4) /* Has Large System Extensions.
>> */
>> #define AARCH64_FL_V8_1 (1 << 5) /* Has ARMv8.1 extensions. */
>> +#define AARCH64_FL_V8_2 (1 << 8) /* Has ARMv8.2 features. */
>> +#define AARCH64_FL_F16 (1 << 9) /* Has ARMv8.2 FP16 extensions.
>> */
>
> Please add a new "section" for the ARMv8.2-A architecture extensions (just
> a new comment that groups them), and s/ARMv8.2/ARMv8.2-A/ throughout this
> section (if you'd like to do the follow-up trivial patch for
> s/ARMv8.1/ARMv8.1-A/ I'd appreciate that).
>
>> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
>> index aa11209..d44bbc0 100644
>> --- a/gcc/doc/invoke.texi
>> +++ b/gcc/doc/invoke.texi
>> @@ -13035,7 +13035,10 @@ more feature modifiers. This option has the form
>> @option{-march=@var{arch}@r{@{}+@r{[}no@r{]}@var{feature}@r{@}*}}.
>>
>> The permissible values for @var{arch} are @samp{armv8-a},
>> -@samp{armv8.1-a} or @var{native}.
>> +@samp{armv8.1-a}, @samp{armv8.2-a} or @var{native}.
>> +
>> +The value @samp{armv8.2-a} implies @samp{armv8.1-a} and enables compiler
>> +support for the ARMv8.2 architecture extensions.
>
> ARMv8.2-A here too please.
Why do we need to say that v8.2-a implies v8.1-a, when we don't say that
v8.1-a implies v8-a? The whole implies clause seems unnecessary to me.
R.
>
>>
>> The value @samp{armv8.1-a} implies @samp{armv8-a} and enables compiler
>> support for the ARMv8.1 architecture extension. In particular, it
>> @@ -13140,6 +13143,8 @@ instructions. This is on by default for all
>> possible values for options
>> @item lse
>> Enable Large System Extension instructions. This is on by default for
>> @option{-march=armv8.1-a}.
>> +@item fp16
>> +Enable FP16 extension.
>
> Please add a note here that enabling this extension also enables "fp"
>
> This is OK for trunk otherwise.
>
> Thanks,
> James
>
>>
>> 2016-06-27 Matthew Wahab <matthew.wa...@arm.com>
>> Jiong Wang <jiong.w...@arm.com>
>>
>> * config/aarch64/aarch64-arches.def: Add "armv8.2-a".
>> * config/aarch64/aarch64.h (AARCH64_FL_V8_2): New.
>> (AARCH64_FL_F16): New.
>> (AARCH64_FL_FOR_ARCH8_2): New.
>> (AARCH64_ISA_8_2): New.
>> (AARCH64_ISA_F16): New.
>> (TARGET_FP_F16INST): New.
>> (TARGET_SIMD_F16INST): New.
>> * config/aarch64/aarch64-option-extensions.def ("fp16"): New entry.
>> ("fp"): Disabling "fp" also disables "fp16".
>> * config/aarch64/aarch64-c.c (arch64_update_cpp_builtins):
>> Conditionally define
>> __ARM_FEATURE_FP16_SCALAR_ARITHMETIC and
>> __ARM_FEATURE_FP16_VECTOR_ARITHMETIC.
>> * doc/invoke.texi (AArch64 Options): Document "armv8.2-a" and "fp16".
>
