RFR: 8345125: Aarch64: Add aarch64 backend for Float16 scalar operations [v2]
Hao Sun
haosun at openjdk.org
Wed Feb 26 14:41:56 UTC 2025
On Wed, 26 Feb 2025 08:49:58 GMT, Bhavana Kilambi <bkilambi at openjdk.org> wrote:
>> src/hotspot/cpu/aarch64/assembler_aarch64.hpp line 2097:
>>
>>> 2095:
>>> 2096: // Half-precision floating-point instructions
>>> 2097: INSN(fabdh, 0b011, 0b11, 0b000101, 0b0);
>>
>> I suppose `fadbh` and `fnmulh` are added to keep aligned with the float and double ones, i.e. `fabd(s|d)` and `fnmul(s|d)`.
>>
>>
>> I noticed that there are matching rules for `fabd(s|d)`, i.e. `absd(F|D)_reg`. I wonder if we need add the corresponding rule for fp16 here?
>
> Hi @shqking , thanks for your review comments. Yes I added `fabdh` and `fnmulh` to keep aligned with float and double types.
> For adding support for FP16 `absd` we need `AbsHF` to be supported (along with SubHF) but `AbsHF` node is not implemented currently. `abs` operation is directly executed from the java code here - https://github.com/openjdk/jdk/blob/037e47112bdf2fa2324f7c58198f6d433f17d9fd/src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Float16.java#L1464 and is not intrinsified or pattern matched like other FP16 operations. Same with `negate` operation for FP16 - https://github.com/openjdk/jdk/blob/037e47112bdf2fa2324f7c58198f6d433f17d9fd/src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Float16.java#L1449
> On the Valhalla repo, while these operation were being developed, I tried adding support for `AbsHF/NegHF` which emitted `fabs` and `fneg` instructions but the performance with the direct java code(bit manipulation operations) was much faster (sorry don't remember the exact number) so we decided to go with the java implementation instead.
> I still added `fabd` here because `op21` is 0 only in `fabd` H variant and felt that it'd be better to handle it here as it belongs to this group of instructions. Please let me know your thoughts.
@Bhavana-Kilambi Thanks for your explanation for the missing `AbsHF`. It's okay to me to have `fadbh` and `fnmulh` in this patch.
Overall it's good to me except aph's comment above.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23748#discussion_r1971712164
More information about the hotspot-dev
mailing list