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 Thanks for your response. And thanks for the list of changes included in the patch :) It seems to me, many of these subtasks you mention could be done as separate tasks prior to the Float16Vector and auto-vectorizer work: 1 Changes to introduce a new (custom) basictype T_FLOAT16 - Global Definition. - Skip over handling where ever applicable. And 2 Changes to pass laneType (BasicType) to intrinsific entry point instead of element classes. - Inline expander interface changes mainly. And in the below at least changes that don't include the Float16Vector: 3 Changes in abstract and concrete vector class generation templates. 4 Changing the nomenclature of Vector classes to avoid Float1664... sort of names... Probably also this: 5 Changes in the LaneType to add a new carrier type field. And maybe also this, as long as it is not Float16 specific: 6 Changes in inline expanders to selectivelty enable intrinsification for opration for which we have auto-vectorization and backend support in place.. For `7`, probably only the `7b` part, since `7a` is about Float16Vector. 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. Parts 8, 9, 10 seem Float16Vector specific, so those should stay here. ------------- PR Comment: https://git.openjdk.org/jdk/pull/28002#issuecomment-3789507594
