On Mon, 20 Nov 2023 01:47:34 GMT, Xiaohong Gong <xg...@openjdk.org> wrote:
>> make/autoconf/lib-vmath.m4 line 92:
>>
>>> 90: []
>>> 91: )
>>> 92: AC_MSG_RESULT([${SVE_FEATURE_SUPPORT}])
>>
>> What is this test even for? I can't see any usage of SVE_FEATURE_SUPPORT
>> outside this function.
>
> This is just used to print the result of `AC_MSG_CEHCKING[if ARM SVE feature
> is supported]` in configure.
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 the `SVE_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 by `FLAGS_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 changing `OPENJDK_TARGET_ABI_PROFILE` can
have further consequences; I do not know the full extent on the top of my head.
>> make/autoconf/libraries.m4 line 129:
>>
>>> 127: LIB_SETUP_LIBFFI
>>> 128: LIB_SETUP_MISC_LIBS
>>> 129: LIB_SETUP_VMATH
>>
>> The function (and file) should be named after "sleef", not "vmath".
>
> Yes, it seems weird. But the library we want to built out is `libvmath.so`
> instead of `libsleef.so`. And we not only check the sleef library, but also
> the ARM SVE feature inside it. So using `VMATH` suffix is more reasonable to
> me. WDYT?
As I said above, you should not mix the two together. Keep the library handling
for libsleef. Move the march setting to where it belongs. And rename the files,
functions and variables after this.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16234#discussion_r1400663476
PR Review Comment: https://git.openjdk.org/jdk/pull/16234#discussion_r1400665010
