On Fri, 5 Apr 2024 12:17:17 GMT, Hamlin Li <m...@openjdk.org> wrote: >> Hi, >> Can you help to review the patch? >> This pr is based on previous work and discussion in [pr >> 16234](https://github.com/openjdk/jdk/pull/16234), [pr >> 18294](https://github.com/openjdk/jdk/pull/18294). >> >> Compared with previous prs, the major change in this pr is to integrate the >> source of sleef (for the steps, please check >> `src/jdk.incubator.vector/linux/native/libvectormath/README`), rather than >> depends on external sleef things (header or lib) at build or run time. >> Besides of this change, also modify the previous changes accordingly, e.g. >> remove some uncessary files or changes especially in make dir of jdk. >> >> Besides of the code changes, one important task is to handle the legal >> process. >> >> Thanks! > > Hamlin Li has updated the pull request incrementally with two additional > commits since the last revision: > > - disable unused-function warnings; add log msg > - minor
Nice work, Hamlin and Xiaohong. I'm glad to see progress on incorporating SLEEF library into the JDK. (Somehow I missed all previous PRs you posted before.) I'm not a lawyer, so won't comment on 3rd party library sources under Boost Software License in OpenJDK. >From engineering perspective, I believe that bundling vector math library with >the JDK is the right thing to do, but it doesn't imply the sources should be >part of JDK. There are already examples of optional dependencies on external >native libraries in HotSpot (e.g., hsdis tool w/ binutils, capstone, and llvm >backends). Speaking of HotSpot-specific changes, IMO it desperately needs a cross-platform interface between vector math libraries and JVM. Most of the changes in `StubGenerator` are library-specific and are irrelevant in the context of the JVM. I do see that you try to replicate SVML logic, but SVML support didn't set a precedent to follow here. For background, SVML stubs were initially contributed to Panama as assembly stubs statically linked into libjvm.so. It was acceptable for experimentation purposes, but not for mainline JDK (even for functionality in incubating module). The compromise was to bundle the stubs as a dynamic library and link against them. And that's how it stayed until today. IMO in order to get SLEEF in, the interaction between JVM and backend native library should be unified. And it should affect both SLEEF and SVML stubs. In particular, I'd like to see all those named lookups to go away from the JVM code. A single call into the library during compiler/VM initialization can produce a fully populated table of function pointers (`StubRoutines::_vector_[fd]_math` now) for C2 to use later. FTR there were other alternatives discussed (use Panama FFI or rewrite the stubs in Vector API itself). The latter (complete rewrite) is still something for a distant future, but Foreign Function API is public API now, so once it supports vector calling conventions, it should become fully capable of satisfying Vector API implementation needs to interact with vector math library. IMO that what we should keep in mind when designing new interface. There's no inherent need to keep vector stub support in the JVM. Once Foreign Function API gains vector support, it should be replaced with a pure Java FFI-based implementation. ------------- PR Comment: https://git.openjdk.org/jdk/pull/18605#issuecomment-2048599089
