-
Notifications
You must be signed in to change notification settings - Fork 5.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8353786: Migrate Vector API math library support to FFM API #24462
base: master
Are you sure you want to change the base?
Conversation
👋 Welcome back vlivanov! A progress list of the required criteria for merging this PR into |
❗ This change is not yet ready to be integrated. |
@iwanowww The |
Moving vector API library selection to Java code looks like a right step to me. |
03c5a8c
to
fc27aee
Compare
/cc hotspot |
@iwanowww |
Webrevs
|
Entry<T> constructEntry(Operator op, int opc, VectorSpecies<E> vspecies, IntFunction<T> implSupplier) { | ||
if (LIBRARY.isSupported(op, vspecies)) { | ||
String symbol = LIBRARY.symbolName(op, vspecies); | ||
MemorySegment addr = LOOKUP.find(symbol) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is better to use LOOKUP.findOrThrow()
because it does not require lambda creation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, changed as you suggested. I introduced a try-catch block instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few questions?
return features.contains(feature.toLowerCase()); | ||
} | ||
|
||
public static class X64 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we create src/jdk.incubator.vector/cpu/
for CPU specific information? As separate refactoring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify: are you suggesting to move platform-specific classes into a separate package or platform-specific location?
It does make sense to separate platform-specific parts into their own classes once amount of code grows over some limit. For now it doesn't look too attractive since amount of code is very small.
} | ||
|
||
/** | ||
* Naming convention in SVML vector math library. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this library has code for all AVX configurations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, there are 4 configurations (-XX:UseAVX=[0..3]
) in total covered by SVML library.
src/jdk.incubator.vector/share/classes/jdk/incubator/vector/CPUFeatures.java
Outdated
Show resolved
Hide resolved
In addition to addressing review feedback, there are 2 updates:
|
Migrate Vector API math library (SVML and SLEEF) linkage from native code (in JVM) to Java FFM API.
Since FFM API doesn't support vector calling conventions yet, migration affects only symbol lookup for now. But it still enables significant simplifications on JVM side.
The patch consists of the following parts:
jdk.incubator.vector.CPUFeatures
) used for CPU dispatching.java.lang.foreign
API is used to perform symbol lookup in vector math library, then the address is cached and fed into corresponding JVM intrinsic, so C2 can turn it into a direct vector call in generated code.Once
java.lang.foreign
supports vectors & vector calling conventions, VM intrinsics can go away.Performance is on par with original implementation (tested with microbenchmarks on linux-x64 and macosx-aarch64).
Testing: hs-tier1 - hs-tier6, microbenchmarks (on linux-x64 and macosx-aarch64)
Thanks!
Progress
Issue
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24462/head:pull/24462
$ git checkout pull/24462
Update a local copy of the PR:
$ git checkout pull/24462
$ git pull https://git.openjdk.org/jdk.git pull/24462/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 24462
View PR using the GUI difftool:
$ git pr show -t 24462
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24462.diff
Using Webrev
Link to Webrev Comment