[lworld+fp16] RFR: 8330021: AArch64: Add backend support for FP16 add operation [v4]

[Jatin Bhateja]

On Fri, 5 Jul 2024 13:58:52 GMT, Bhavana Kilambi <bkilambi at openjdk.org> wrote:

>> src/hotspot/share/opto/vectornode.cpp line 595:
>> 
>>> 593:   case Op_ConvF2HF:
>>> 594:   case Op_ReinterpretS2HF:
>>> 595:   case Op_ReinterpretHF2S:
>> 
>> ReinterpretHF2S carry a short type, does not qualify as float16 node.
>
> I actually stumbled upon a test failure when I was testing this pattern - `dst (ConvHF2F (ReinterpretHF2S src))` using this test case - 
>  ```
> // fin[] -> Float16 array
> // flout[] -> Float array
> public static void fp16Add() {
>         for (int i = 0; i < SIZE; i++) {
>             flout[i] = Float16.sum(fin[i], fin[i]).floatValue();
> }
> 
> 
> and with a debug build the test fails with the following error -
> 
> 
> #
> # A fatal error has been detected by the Java Runtime Environment:
> #
> #  Internal Error (/home/bhakil01/valhalla_gerrit/valhalla/src/hotspot/share/opto/vectornode.cpp:1423), pid=2559308, tid=2559322
> #  Error: assert(bt == T_SHORT) failed
> #
> # JRE version: OpenJDK Runtime Environment (23.0) (fastdebug build 23-internal-adhoc.bhakil01.valhalla)
> # Java VM: OpenJDK 64-Bit Server VM (fastdebug 23-internal-adhoc.bhakil01.valhalla, mixed mode, compressed oops, compressed class ptrs, g1 gc, linux-aarch64)
> # Problematic frame:
> # V  [libjvm.so+0x18d00d8]  VectorCastNode::opcode(int, BasicType, bool)+0xf4
> #
> 
> 
> This happens because during auto-vectorization, the container type of `ReinterpretHF2S` is computed as `T_INT` and not `T_SHORT` while the input to ConvHF2F is supposed to be T_SHORT which is why the assertion fails here - https://github.com/openjdk/valhalla/blob/779d77f04810c137bb6a3374608b80cbe5cc9ef3/src/hotspot/share/opto/vectornode.cpp#L1422
> 
> The container type for `ReinterpretHF2S` is computed as `TypeInt::INT` here - https://github.com/openjdk/valhalla/blob/779d77f04810c137bb6a3374608b80cbe5cc9ef3/src/hotspot/share/opto/superword.cpp#L3352 which is propagating down to the VectorCastNode and leading to the assertion failure.
> 
> So I have included this node in `is_float16_node()` to return `TypeInt::SHORT` for `ReinterpretHF2S`.
> 
> If you feel this is not the right place for this node, maybe I can do something like - 
> 
> 
> if (VectorNode::is_float16_node(n->Opcode()) || Op_ReinterpretHF2S) {
>     return TypeInt::SHORT;
>   }

Thanks for the explanation, we hit similar issue earlier and [JDK-8333890](https://bugs.openjdk.org/browse/JDK-8333890?filter=-3) tracks it. I agree with above fix to move ReinterpretHF2S out of is_float16_node for the time being.

Kindly add a test for the same.

-------------

PR Review Comment: https://git.openjdk.org/valhalla/pull/1096#discussion_r1667001698