Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
8312425: [vectorapi] AArch64: Optimize vector math operations with SLEEF #16234
8312425: [vectorapi] AArch64: Optimize vector math operations with SLEEF #16234
Changes from 5 commits
2a7730d
286ef3a
f2098d4
efdaa81
b29df84
2c3c4a6
c1ce196
af23b5d
ee5caf6
f3ff067
1463952
c55357b
7a4be73
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
What's this about? We're building a standard binary, and all SVE detection is to be deferred to runtime.
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.
We have to use this c-compiler option to build out the SVE ABIs (e.g.
svfloat32_t sinfx_u10sve(svfloat32_t input)
) inlibvmath.so
. Without this option, at build time, all the sve related featues likearm_sve.h / __ARM_FEATURE_SVE
are missing, together with the sve symbols inlibvmath.so
we needed at runtime. That means at runtime, hotspot cannot find out the sve symbols and the vector operations will fall back to the default java implementation.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.
That's fine, but we must make sure that SVE is not used by the compiler in any other places. If you've already built on non-SVE and tested the result on both SVE and non-SVE, I'm happy.
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.
You still need to separate out the SVE detection from the libsleef code, and provide a way to enable/disable it from the configure command line. It is not okay to auto-detect if features should be turned on or off by default, but it should always be possible to override.
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.
Why? I don't think this should be a build-time option at all, because it puts the people who build binaries in an impossible position. Can't this all be built unconditionally, with run-time feature detection?
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.
Apparently the situation is this: If your build machine happens to have SVE, then you will get SVE support in the vmath library. The SVE support will be used during runtime if the machine you run on has SVE support.
If your build host happens to to not have SVE, then the vmath library will be built without SVE support, and no matter if your runtime machine has SVE or not, it will not provide SVE support in the vmath library.
Now, if your CI farm has an arbitrarily selection of aarch64 machines with and without SVE, then you have no idea what you are going to get in your build.
We have been very careful in staying clear of this kind of "random" build system behavior. The system you build on should not affect the output -- at least, not without a chance to override the default value.
In fact, I am not even sure why it seems to the PR author to be a good idea to let the default be dependent on the build machine at all. My personal opinion is that it would be better to select either "SVE enabled" or "SVE disabled" as the default, and then let the user override this on the configure command line, if they target a platform with different SVE availability.
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.
SVE support should be enabled regardless of the build machine. The same binary must run on both SVE and non-SVE machines, using SVE if it is advantageous. I suppose some ancient C++ compilers without SVE support still exist, but I see no very good reason to support them building JDK 22+.
Making a configure option to disable SVE support for vector math is a mistake, but IMO mostly harmless because no-one will ever turn it off.
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.
Agree.
Since it contains both NEON and SVE functions in libvmath.so, we have to disable SVE feature when building those NEON functions. We want to separate NEON/SVE functions in two files, build them with different cflags (i.e. only build SVE sources with
-march=armv8-a+sve
), and then link to the singlelibvmath.so
. Do we have such similar examples or functions in current jdk make system? I'm still struggling on finding out an effective way for it.Yes, SVE feature is also always enabled in jdk hotspot on SVE machines. If we add the option to give people disable SVE, it's weird that we disabling the SVE just in libvmath.so, and enabling it in hotspot. Besides, we choose the NEON stubs for smaller than 128-bit vector operations no matter whether the runtime machine supports SVE or not. So performance may not be an issue. Hence, I don't think people have reason disabling SVE features.
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 makes no sense to configure any of this at build time. Postpone all of the decisions to runtime.
Don't touch the make system.Instead, try to open the library at runtime with
os::dll_open()
, after (or inside)VM_Version::initialize()
.If you're not running on an SVE system, none of the SVE routines will be called, so it doesn't matter, right?
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.
What is this test even for? I can't see any usage of SVE_FEATURE_SUPPORT outside this function.
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.
This is just used to print the result of
AC_MSG_CEHCKING[if ARM SVE feature is supported]
in configure.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.
Ah, now I se what you are trying to do here. First of all, in the detection part, only set
SVE_FEATURE_SUPPORT
. Then you can handle theSVE_CFLAGS
addition elsewhere/later.Secondly, you should not mix these
SVE_CFLAGS
with the spleef C flags. Keeping them separate will allow for LIBSLEEF_CFLAGS to be named just that.Thirdly, I do not like at all how you just come crashing in setting
-march
like that. The-march
flag is handled byFLAGS_SETUP_ABI_PROFILE
.Actually, now that I think of it, this is just completely wrong! You are checking on features on the build machine, to determine what target machine code to generate, with no way to override.
You need to break out the -march handling separately. It should be moved to FLAGS_SETUP_ABI_PROFILE. I'm guessing you will need to make something like a
aarch64-sve
profile, and possibly try to auto-select it based on the result of the sve test program above. But changingOPENJDK_TARGET_ABI_PROFILE
can have further consequences; I do not know the full extent on the top of my head.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 for the advice! I will take a consideration for it.
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.
-march=armv8-a+sve
is just used in this new added module, which may not expect to be used for other libraries. Per my understanding, flags handled byFLAGS_SETUP_ABI_PROFILE
is not used for a specified module?Yes, that's be a risk, although the usage to the SVE functions are controlled by SVE feature as well in runtime. I need time to find a better solution.
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 does not matter if you set the -march on just part of the build. Actually, there is no point in doing so. Either the JDK is run on a machine with the matching architecture, or it isn't.
I don't know the details of what the aarch64 SVE feature means, but unless this is a special instance, any attempt to execute the compiled code on a machine that does not support that architecture will fail.