diff mbox series

[REVIEW,ONLY,4/4] RISC-V: Tentative ".bfloat16" assembly support

Message ID 7fbfd7ca7fdc34afc0111af0f7beb7c69839fc04.1691021079.git.research_trasio@irq.a4lg.com
State Unresolved
Headers
Series UNRATIFIED RISC-V: Add support for BFloat16 extensions |

Checks

Context Check Description
snail/binutils-gdb-check warning Git am fail log

Commit Message

Tsukasa OI Aug. 3, 2023, 12:04 a.m. UTC 
  From: Tsukasa OI <research_trasio@irq.a4lg.com>

This commit adds an assembler directive ".bfloat16" to help BFloat16
extensions ('Zfbfmin', 'Zvfbfmin' and 'Zvfbfwma') users.

gas/ChangeLog:

	* config/tc-riscv.c (FLT_CHARS): Add BFloat16 'b' to supported
	floating point formats.
	(riscv_pseudo_table) Add ".bfloat16" directive.
	* testsuite/gas/riscv/bfloat16.s: Copied from
	testsuite/gas/aarch64/bfloat16-directive.s.
	* testsuite/gas/riscv/bfloat16-be.d: New test ported from
	testsuite/gas/aarch64/bfloat16-directive-be.d and float16-be.d.
	* testsuite/gas/riscv/bfloat16-le.d: New test ported from
	testsuite/gas/aarch64/bfloat16-directive-le.d and float16-le.d.
---
 gas/config/tc-riscv.c                 |  3 ++-
 gas/testsuite/gas/riscv/bfloat16-be.d | 10 ++++++++++
 gas/testsuite/gas/riscv/bfloat16-le.d | 10 ++++++++++
 gas/testsuite/gas/riscv/bfloat16.s    | 19 +++++++++++++++++++
 4 files changed, 41 insertions(+), 1 deletion(-)
 create mode 100644 gas/testsuite/gas/riscv/bfloat16-be.d
 create mode 100644 gas/testsuite/gas/riscv/bfloat16-le.d
 create mode 100644 gas/testsuite/gas/riscv/bfloat16.s

Comments

Jan Beulich Aug. 3, 2023, 6:47 a.m. UTC | #1 
On 03.08.2023 02:04, Tsukasa OI via Binutils wrote:> --- a/gas/config/tc-riscv.c
> +++ b/gas/config/tc-riscv.c
> @@ -437,7 +437,7 @@ const char EXP_CHARS[] = "eE";
>  
>  /* Chars that mean this number is a floating point constant.
>     As in 0f12.456 or 0d1.2345e12.  */
> -const char FLT_CHARS[] = "rRsSfFdDxXpPhH";
> +const char FLT_CHARS[] = "rRsSfFdDxXpPhHb";

I realize Arm64 also has it this way (x86 doesn't), but are you sure
about only adding 'b' and not also 'B'? Aiui this introduces a needless
special case of case sensitivity.

Jan
Tsukasa OI Aug. 3, 2023, 7:17 a.m. UTC | #2 
On 2023/08/03 15:47, Jan Beulich wrote:
> On 03.08.2023 02:04, Tsukasa OI via Binutils wrote:> --- a/gas/config/tc-riscv.c
>> +++ b/gas/config/tc-riscv.c
>> @@ -437,7 +437,7 @@ const char EXP_CHARS[] = "eE";
>>  
>>  /* Chars that mean this number is a floating point constant.
>>     As in 0f12.456 or 0d1.2345e12.  */
>> -const char FLT_CHARS[] = "rRsSfFdDxXpPhH";
>> +const char FLT_CHARS[] = "rRsSfFdDxXpPhHb";
> 
> I realize Arm64 also has it this way (x86 doesn't), but are you sure
> about only adding 'b' and not also 'B'? Aiui this introduces a needless
> special case of case sensitivity.
> 
> Jan
> 

I'm not sure (the only reason I did so is because what I saw is AArch64
code).  So, I'll investigate whether introducing additional character
will benefit (I think there's not so many cases but there might be at
least one).

Tsukasa
Tsukasa OI Aug. 3, 2023, 7:51 a.m. UTC | #3 
On 2023/08/03 16:17, Tsukasa OI via Binutils wrote:
> 
> 
> On 2023/08/03 15:47, Jan Beulich wrote:
>> On 03.08.2023 02:04, Tsukasa OI via Binutils wrote:> --- a/gas/config/tc-riscv.c
>>> +++ b/gas/config/tc-riscv.c
>>> @@ -437,7 +437,7 @@ const char EXP_CHARS[] = "eE";
>>>  
>>>  /* Chars that mean this number is a floating point constant.
>>>     As in 0f12.456 or 0d1.2345e12.  */
>>> -const char FLT_CHARS[] = "rRsSfFdDxXpPhH";
>>> +const char FLT_CHARS[] = "rRsSfFdDxXpPhHb";
>>
>> I realize Arm64 also has it this way (x86 doesn't), but are you sure
>> about only adding 'b' and not also 'B'? Aiui this introduces a needless
>> special case of case sensitivity.
>>
>> Jan
>>
> 
> I'm not sure (the only reason I did so is because what I saw is AArch64
> code).  So, I'll investigate whether introducing additional character
> will benefit (I think there's not so many cases but there might be at
> least one).
> 
> Tsukasa
> 

I checked the code and actually, no code is affected by lacking 'B'.  I
first thought something like 0B:1234 (raw hexadecimal representation as
float) is affected but in a floating number literal, any 0[a-zA-Z]:1234
turns to a 16-bit word 0x1234, reinterpreted as a floating point number
(and the second character is just ignored and the length is checked
against another parameter).

So, lacking 'B' here is no problem for us and adding 'B' will be rather
confusing (than just 'b').

Thanks,
Tsukasa
Jan Beulich Aug. 3, 2023, 8 a.m. UTC | #4 
On 03.08.2023 09:51, Tsukasa OI wrote:
> On 2023/08/03 16:17, Tsukasa OI via Binutils wrote:
>>
>>
>> On 2023/08/03 15:47, Jan Beulich wrote:
>>> On 03.08.2023 02:04, Tsukasa OI via Binutils wrote:> --- a/gas/config/tc-riscv.c
>>>> +++ b/gas/config/tc-riscv.c
>>>> @@ -437,7 +437,7 @@ const char EXP_CHARS[] = "eE";
>>>>  
>>>>  /* Chars that mean this number is a floating point constant.
>>>>     As in 0f12.456 or 0d1.2345e12.  */
>>>> -const char FLT_CHARS[] = "rRsSfFdDxXpPhH";
>>>> +const char FLT_CHARS[] = "rRsSfFdDxXpPhHb";
>>>
>>> I realize Arm64 also has it this way (x86 doesn't), but are you sure
>>> about only adding 'b' and not also 'B'? Aiui this introduces a needless
>>> special case of case sensitivity.
>>
>> I'm not sure (the only reason I did so is because what I saw is AArch64
>> code).  So, I'll investigate whether introducing additional character
>> will benefit (I think there's not so many cases but there might be at
>> least one).
> 
> I checked the code and actually, no code is affected by lacking 'B'.  I
> first thought something like 0B:1234 (raw hexadecimal representation as
> float) is affected but in a floating number literal, any 0[a-zA-Z]:1234
> turns to a 16-bit word 0x1234, reinterpreted as a floating point number
> (and the second character is just ignored and the length is checked
> against another parameter).

I'm actually questioning that overly lax check; IOW I wonder whether it
shouldn't consult FLT_CHARS[].

Jan
Tsukasa OI Aug. 3, 2023, 8:21 a.m. UTC | #5 
On 2023/08/03 17:00, Jan Beulich via Binutils wrote:
> On 03.08.2023 09:51, Tsukasa OI wrote:
>> On 2023/08/03 16:17, Tsukasa OI via Binutils wrote:
>>>
>>>
>>> On 2023/08/03 15:47, Jan Beulich wrote:
>>>> On 03.08.2023 02:04, Tsukasa OI via Binutils wrote:> --- a/gas/config/tc-riscv.c
>>>>> +++ b/gas/config/tc-riscv.c
>>>>> @@ -437,7 +437,7 @@ const char EXP_CHARS[] = "eE";
>>>>>  
>>>>>  /* Chars that mean this number is a floating point constant.
>>>>>     As in 0f12.456 or 0d1.2345e12.  */
>>>>> -const char FLT_CHARS[] = "rRsSfFdDxXpPhH";
>>>>> +const char FLT_CHARS[] = "rRsSfFdDxXpPhHb";
>>>>
>>>> I realize Arm64 also has it this way (x86 doesn't), but are you sure
>>>> about only adding 'b' and not also 'B'? Aiui this introduces a needless
>>>> special case of case sensitivity.
>>>
>>> I'm not sure (the only reason I did so is because what I saw is AArch64
>>> code).  So, I'll investigate whether introducing additional character
>>> will benefit (I think there's not so many cases but there might be at
>>> least one).
>>
>> I checked the code and actually, no code is affected by lacking 'B'.  I
>> first thought something like 0B:1234 (raw hexadecimal representation as
>> float) is affected but in a floating number literal, any 0[a-zA-Z]:1234
>> turns to a 16-bit word 0x1234, reinterpreted as a floating point number
>> (and the second character is just ignored and the length is checked
>> against another parameter).
> 
> I'm actually questioning that overly lax check; IOW I wonder whether it
> shouldn't consult FLT_CHARS[].
> 
> Jan
> 

Hmm, I'm tempted to say "don't ask me".  I mean, that overly lax check
is in the portable portion of GAS and not RISC-V-specific.  I will start
digging if I'm interested... but currently... I'm not quite sure.

Regards,
Tsukasa
Jan Beulich Aug. 3, 2023, 8:25 a.m. UTC | #6 
On 03.08.2023 10:21, Tsukasa OI wrote:
> On 2023/08/03 17:00, Jan Beulich via Binutils wrote:
>> On 03.08.2023 09:51, Tsukasa OI wrote:
>>> On 2023/08/03 16:17, Tsukasa OI via Binutils wrote:
>>>>
>>>>
>>>> On 2023/08/03 15:47, Jan Beulich wrote:
>>>>> On 03.08.2023 02:04, Tsukasa OI via Binutils wrote:> --- a/gas/config/tc-riscv.c
>>>>>> +++ b/gas/config/tc-riscv.c
>>>>>> @@ -437,7 +437,7 @@ const char EXP_CHARS[] = "eE";
>>>>>>  
>>>>>>  /* Chars that mean this number is a floating point constant.
>>>>>>     As in 0f12.456 or 0d1.2345e12.  */
>>>>>> -const char FLT_CHARS[] = "rRsSfFdDxXpPhH";
>>>>>> +const char FLT_CHARS[] = "rRsSfFdDxXpPhHb";
>>>>>
>>>>> I realize Arm64 also has it this way (x86 doesn't), but are you sure
>>>>> about only adding 'b' and not also 'B'? Aiui this introduces a needless
>>>>> special case of case sensitivity.
>>>>
>>>> I'm not sure (the only reason I did so is because what I saw is AArch64
>>>> code).  So, I'll investigate whether introducing additional character
>>>> will benefit (I think there's not so many cases but there might be at
>>>> least one).
>>>
>>> I checked the code and actually, no code is affected by lacking 'B'.  I
>>> first thought something like 0B:1234 (raw hexadecimal representation as
>>> float) is affected but in a floating number literal, any 0[a-zA-Z]:1234
>>> turns to a 16-bit word 0x1234, reinterpreted as a floating point number
>>> (and the second character is just ignored and the length is checked
>>> against another parameter).
>>
>> I'm actually questioning that overly lax check; IOW I wonder whether it
>> shouldn't consult FLT_CHARS[].
> 
> Hmm, I'm tempted to say "don't ask me".  I mean, that overly lax check
> is in the portable portion of GAS and not RISC-V-specific.

Of course. My remark went towards: Wouldn't it be better if targets weren't
dependent on that overlay lax check? (Interestingly Arm32, despite
supporting bfloat16, looks to have neither b nor B in it FLT_CHARS[], and
is - for now - still getting away with that.)

Jan
diff mbox series

Patch

diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
index aaf8b9be64fd..9f15233c7b99 100644
--- a/gas/config/tc-riscv.c
+++ b/gas/config/tc-riscv.c
@@ -437,7 +437,7 @@  const char EXP_CHARS[] = "eE";
 
 /* Chars that mean this number is a floating point constant.
    As in 0f12.456 or 0d1.2345e12.  */
-const char FLT_CHARS[] = "rRsSfFdDxXpPhH";
+const char FLT_CHARS[] = "rRsSfFdDxXpPhHb";
 
 /* Indicate we are already assemble any instructions or not.  */
 static bool start_assemble = false;
@@ -5178,6 +5178,7 @@  static const pseudo_typeS riscv_pseudo_table[] =
   {"attribute", s_riscv_attribute, 0},
   {"variant_cc", s_variant_cc, 0},
   {"float16", float_cons, 'h'},
+  {"bfloat16", float_cons, 'b'},
 
   { NULL, NULL, 0 },
 };
diff --git a/gas/testsuite/gas/riscv/bfloat16-be.d b/gas/testsuite/gas/riscv/bfloat16-be.d
new file mode 100644
index 000000000000..8775ab035f6c
--- /dev/null
+++ b/gas/testsuite/gas/riscv/bfloat16-be.d
@@ -0,0 +1,10 @@ 
+# source: bfloat16.s
+# objdump: -sj .data
+# as: -mbig-endian
+
+.*: +file format .*
+
+Contents of section \.data:
+ 0000 41403dfc 000042f7 8000c2f7 7fff7f80  .*
+ 0010 ff807f7f ff7f0080 80800001 8001007f  .*
+ 0020 807f3f80 bf804000 c000.*
diff --git a/gas/testsuite/gas/riscv/bfloat16-le.d b/gas/testsuite/gas/riscv/bfloat16-le.d
new file mode 100644
index 000000000000..4de8b2fed68b
--- /dev/null
+++ b/gas/testsuite/gas/riscv/bfloat16-le.d
@@ -0,0 +1,10 @@ 
+# source: bfloat16.s
+# objdump: -sj .data
+# as: -mlittle-endian
+
+.*: +file format .*
+
+Contents of section \.data:
+ 0000 4041fc3d 0000f742 0080f7c2 ff7f807f  .*
+ 0010 80ff7f7f 7fff8000 80800100 01807f00  .*
+ 0020 7f80803f 80bf0040 00c0.*
diff --git a/gas/testsuite/gas/riscv/bfloat16.s b/gas/testsuite/gas/riscv/bfloat16.s
new file mode 100644
index 000000000000..66e17e4fc7da
--- /dev/null
+++ b/gas/testsuite/gas/riscv/bfloat16.s
@@ -0,0 +1,19 @@ 
+.data
+	.bfloat16 12.0
+	.bfloat16 0.123
+	.bfloat16 +0.0
+	.bfloat16 123.4
+	.bfloat16 -0.0
+	.bfloat16 -123.4
+	.bfloat16 NaN
+	.bfloat16 Inf
+	.bfloat16 -Inf
+	.bfloat16 3.390e+38
+	.bfloat16 -3.390e+38
+	.bfloat16 1.175e-38
+	.bfloat16 -1.175e-38
+	.bfloat16 9.194e-41
+	.bfloat16 -9.194e-41
+	.bfloat16 1.167e-38
+	.bfloat16 -1.167e-38
+	.bfloat16 1.0, -1, 2.0, -2