On Fri, 23 Jan 2026 04:57:04 GMT, Jatin Bhateja <[email protected]> wrote:
>> Hi @eme64 , Your comments have been addressed > >> @jatin-bhateja This patch is really really large. There are lots of >> renamings that could be done in a separate patch first (as a subtask). It >> would make reviewing easier, allowing focus on the substantial work. See >> discussion here: [#28002 >> (comment)](https://github.com/openjdk/jdk/pull/28002#discussion_r2705376899) > > Hi @eme64 , > > I have done some cleanups, following is the summary of changes included with > the patch:- > > ``` > 1 Changes to introduce a new (custom) basictype T_FLOAT16 > - Global Definition. > - Skip over handling where ever applicable. > 2 Changes to pass laneType (BasicType) to intrinsific entry point instead > of element classes. > - Inline expander interface changes mainly. > 3 Changes in abstract and concrete vector class generation templates. > 4 Changing the nomenclature of Vector classes to avoid Float1664... sort of > names... > 5 Changes in the LaneType to add a new carrier type field. > 6 Changes in inline expanders to selectivelty enable intrinsification for > opration for which we have > auto-vectorization and backend support in place.. > 7 Changes in test generation templates. > b. Assert wrappers to conver float16 (short) value to float before > invoking testng Asserts. > c. Scalar operation wrappers to selectivelty invoke Float16 math > routine which are not > part of Java SE math libraries. > > 8 New IR verification test. > 9 New Micro-benchmark. > 10 AARCH64 test failure - patch + test fixed by Bhavana Kilambi. > > > Out of above change 7b consumes 40000+ LOC. > > Q. Why do we need wrapper assertions ? > A. To handle all possible NaN representations of SNaN and QNaN, since > float16 uses short carrier type hence we need to promote them float values > before invoking TestNG assertions. This conversion is accomplished by > assertion wrappers > > All the tasks are related and most of source/test are generated using scripts > we should not go by the size of patch and review the templates files. > @jatin-bhateja I was wondering: what prompted the decision to add a new > `BasicType` for `Float16`? Would each additional numeric type get a new > `BasicType`? How many do we anticipate? > > Currently, we are using `T_SHORT` for `Float16`, right? Hi @eme64 , Currently in JDK mainline we pass element class as the lane type, problem with passing Float16.class is that its part of incubating module an we cannot declare a symbol for it in vmSymbols, thus if we pass Float16.class as element type we will need to do a fragile name based checks over element type to infer Float16 operation in inline expanders. To circumvent this problem started passing additional integer argument vector operation type (VECTOR_TYPE_FP16 / VECTOR_TYPE_PRIM) to intrinsic entry point. Paul suggested in his [prior comment](https://github.com/openjdk/jdk/pull/28002#issuecomment-3529452461) to add a new basicType for Float16 and instead of passing element class and vector operation type pass just the basicType since its already used in the LaneType. [Enum definitions of all the primitive basic types used in LaneType ](https://urldefense.com/v3/__https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/jdk/internal/vm/vector/VectorSupport.java*L153__;Iw!!ACWV5N9M2RV99hQ!J4ZZ1lwCxaG8mXxtjHB9uET0tlcqBdgJwsC3pCLt4WeUQYULtKPtxo_2NIJw67AYBe6k9ffftGh_EttPe1bY_kYW$) are as per JVM specification and in synchronism with [BasicType definition in VM side](https://urldefense.com/v3/__https://github.com/openjdk/jdk/blob/master/src/hotspot/share/utilities/globalDefinitions.hpp*L671__;Iw!!ACWV5N9M2RV99hQ!J4ZZ1lwCxaG8mXxtjHB9uET0tlcqBdgJwsC3pCLt4WeUQYULtKPtxo_2NIJw67AYBe6k9ffftGh_EttPe6e5uGFc$). VM also defines some custom basic types like T_METADATA, T_NARROWKLASS. If we just create new basic type on Java side, then there is a chance that its value may conflict with existing custom basic types in VM side. One solution is to maintain the synchronization b/w basic type assignment for primitive type only and not modify any VM side code since current scope of T_FLOAT16 is only limited to intrinsic entry point. Adding a new custom BasicType on VM side is not just a change in one place and is cumbersome and not desirable given that its used all across VM code. Thus there are following options :- 1/ Create new basicType T_FLOAT16 in Java side, add it to LaneType and pass only basic types as element type to intrinsic entry point and maintain an efficient interface 2/ Pass Float16.class as element type to Float16Vector operations and do a fragile and inefficient name base lookup in inline expander to infer Float16 vector IR. 3/ Extend both BasicType definition on Java side and VM side and keep them in synchronism but this is not desirable given that VM makes extensive use of BasicType. 4/ Pass short.class as element type and pass another argument vector operation kind to intrinsic entry point to differentiate b/w ShortVector and Float16Vector operations. 5/ Paul's suggestion to create proxy class in java.base module for Float16 type. I am inclined to go with solution 1, let me know if you have other solutions. ------------- PR Comment: https://git.openjdk.org/jdk/pull/28002#issuecomment-3803701515
