8268643: SVML lib shouldn't be generated when C2 is absent

[DamonFool]

Hi all,

SVML lib shouldn't be generated when C2 is absent.
This is because it is only used by C2.

Thanks.
Best regards,
Jie

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8268643: SVML lib shouldn't be generated when C2 is absent

Reviewers

• Erik Joelsson (@erikj79 - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk17 pull/37/head:pull/37
$ git checkout pull/37

Update a local copy of the PR:
$ git checkout pull/37
$ git pull https://git.openjdk.java.net/jdk17 pull/37/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 37

View PR using the GUI difftool:
$ git pr show -t 37

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk17/pull/37.diff

[bridgekeeper]

👋 Welcome back jiefu! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@DamonFool The following label will be automatically applied to this pull request:

• build

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 01: Full - Incremental (cd2549c4)
• 00: Full (0936a8d1)

[erikj79]

I would recommend inverting this logic so that it works properly in the (unlikely) even that two or more JVM variants are built into the same JDK. I assume we want this library built if any JVM variant has c2.

[DamonFool]

Thanks @erikj79 for your review.

What do you mean by two or more JVM variants are built into the same JDK ?
Did you mean two different JVM variants can be built into just one JDK images?

As you suggested, I used to try to use $(call check-jvm-feature, compiler2) like this

diff --git a/make/modules/jdk.incubator.vector/Lib.gmk b/make/modules/jdk.incubator.vector/Lib.gmk
index 2626065..a176b9a 100644
--- a/make/modules/jdk.incubator.vector/Lib.gmk
+++ b/make/modules/jdk.incubator.vector/Lib.gmk
@@ -27,7 +27,7 @@ include LibCommon.gmk
 
 ################################################################################
 
-ifeq ($(call isTargetOs, linux windows)+$(call isTargetCpu, x86_64), true+true)
+ifeq ($(call isTargetOs, linux windows)+$(call isTargetCpu, x86_64)+$(call check-jvm-feature, compiler2), true+true+true)
   $(eval $(call SetupJdkLibrary, BUILD_LIBSVML, \
       NAME := svml, \
       CFLAGS := $(CFLAGS_JDKLIB), \

But it doesn't work.
Am I missing something?

Thanks.

[mlbridge]

Mailing list message from erik.joelsson at oracle.com on build-dev:

On 2021-06-14 06:13, Jie Fu wrote:

On Mon, 14 Jun 2021 12:54:33 GMT, Erik Joelsson <erikj at openjdk.org> wrote:

Hi all,

SVML lib shouldn't be generated when C2 is absent.
This is because it is only used by C2.

Thanks.
Best regards,
Jie
make/autoconf/jvm-features.m4 line 571:

569: ENABLE_CDS="true"
570: INCLUDE_JVMCI="true"
571: INCLUDE_COMPILER2="true"
I would recommend inverting this logic so that it works properly in the (unlikely) even that two or more JVM variants are built into the same JDK. I assume we want this library built if any JVM variant has c2.
Thanks @erikj79 for your review.

What do you mean by `two or more JVM variants are built into the same JDK` ?
Did you mean two different JVM variants can be built into just one JDK images?

Yes, there is still support for building multiple variants into the same
image even though this is not something we currently do for any Oracle
offering. But as long as the build supports it, we should try to keep it
working. In this case your new variable should be true if any variant
that is being built has the compiler2 feature. The current proposal will
skip building the library if any variant does not contain the compiler2
feature. If you initiate the variable to false and change it to true if
a variant has compiler2, we are set.

As you suggested, I used to try to use `$(call check-jvm-feature, compiler2)` like this

diff --git a/make/modules/jdk.incubator.vector/Lib.gmk b/make/modules/jdk.incubator.vector/Lib.gmk
index 2626065..a176b9a 100644
--- a/make/modules/jdk.incubator.vector/Lib.gmk
+++ b/make/modules/jdk.incubator.vector/Lib.gmk
@@ -27,7 +27,7 @@ include LibCommon.gmk

################################################################################

-ifeq ($(call isTargetOs, linux windows)+$(call isTargetCpu, x86_64), true+true)
+ifeq ($(call isTargetOs, linux windows)+$(call isTargetCpu, x86_64)+$(call check-jvm-feature, compiler2), true+true+true)
$(eval $(call SetupJdkLibrary, BUILD_LIBSVML, \
NAME := svml, \
CFLAGS := $(CFLAGS_JDKLIB), \

But it doesn't work.
Am I missing something?

I deleted that comment because it was wrong. The check-jvm-feature macro
only works in the hotspot part of the build where the JVM_VARIANT
variable is set. Those makefiles are called for each JVM_VARIANT being
built.

/Erik

[mlbridge]

Mailing list message from erik.joelsson at oracle.com on build-dev:

On 2021-06-14 06:13, Jie Fu wrote:

On Mon, 14 Jun 2021 12:54:33 GMT, Erik Joelsson <erikj at openjdk.org> wrote:

Hi all,

SVML lib shouldn't be generated when C2 is absent.
This is because it is only used by C2.

Thanks.
Best regards,
Jie
make/autoconf/jvm-features.m4 line 571:

569: ENABLE_CDS="true"
570: INCLUDE_JVMCI="true"
571: INCLUDE_COMPILER2="true"
I would recommend inverting this logic so that it works properly in the (unlikely) even that two or more JVM variants are built into the same JDK. I assume we want this library built if any JVM variant has c2.
Thanks @erikj79 for your review.

What do you mean by `two or more JVM variants are built into the same JDK` ?
Did you mean two different JVM variants can be built into just one JDK images?

Yes, there is still support for building multiple variants into the same
image even though this is not something we currently do for any Oracle
offering. But as long as the build supports it, we should try to keep it
working. In this case your new variable should be true if any variant
that is being built has the compiler2 feature. The current proposal will
skip building the library if any variant does not contain the compiler2
feature. If you initiate the variable to false and change it to true if
a variant has compiler2, we are set.

As you suggested, I used to try to use `$(call check-jvm-feature, compiler2)` like this

diff --git a/make/modules/jdk.incubator.vector/Lib.gmk b/make/modules/jdk.incubator.vector/Lib.gmk
index 2626065..a176b9a 100644
--- a/make/modules/jdk.incubator.vector/Lib.gmk
+++ b/make/modules/jdk.incubator.vector/Lib.gmk
@@ -27,7 +27,7 @@ include LibCommon.gmk

################################################################################

-ifeq ($(call isTargetOs, linux windows)+$(call isTargetCpu, x86_64), true+true)
+ifeq ($(call isTargetOs, linux windows)+$(call isTargetCpu, x86_64)+$(call check-jvm-feature, compiler2), true+true+true)
$(eval $(call SetupJdkLibrary, BUILD_LIBSVML, \
NAME := svml, \
CFLAGS := $(CFLAGS_JDKLIB), \

But it doesn't work.
Am I missing something?

I deleted that comment because it was wrong. The check-jvm-feature macro
only works in the hotspot part of the build where the JVM_VARIANT
variable is set. Those makefiles are called for each JVM_VARIANT being
built.

/Erik

[DamonFool]

Yes, there is still support for building multiple variants into the same
image even though this is not something we currently do for any Oracle
offering.

Sounds amazing.
Updated.
Thanks.

[openjdk]

@DamonFool This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8268643: SVML lib shouldn't be generated when C2 is absent

Reviewed-by: erikj

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 4 new commits pushed to the master branch:

• c088d09: 8268522: InstanceKlass::can_be_verified_at_dumptime() returns opposite value
• abe20c1: 8268333: javac crashes when pattern matching switch contains default case which is not last
• b318535: 8267579: Thread::cooked_allocated_bytes() hits assert(left >= right) failed: avoid underflow
• fe48ea9: 8268342: java/foreign/channels/TestAsyncSocketChannels.java fails with "IllegalStateException: This segment is already closed"

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[DamonFool]

/integrate

[openjdk]

Going to push as commit 4a6da99.
Since your change was applied there have been 9 commits pushed to the master branch:

• a5bf5e0: 8266614: update manpage for -Xlog:async
• f69e2d5: 8267397: AlgorithmId's OID cache is never refreshed
• ee30159: 8268621: SunJCE provider may throw unexpected NPE for un-initialized AES KW/KWP Ciphers
• 702e3ff: 8268366: Incorrect calculation of has_fpu_registers in C1 linear scan
• bca914b: 8268670: yield statements doesn't allow ~ or ! unary operators in expression
• c088d09: 8268522: InstanceKlass::can_be_verified_at_dumptime() returns opposite value
• abe20c1: 8268333: javac crashes when pattern matching switch contains default case which is not last
• b318535: 8267579: Thread::cooked_allocated_bytes() hits assert(left >= right) failed: avoid underflow
• fe48ea9: 8268342: java/foreign/channels/TestAsyncSocketChannels.java fails with "IllegalStateException: This segment is already closed"

Your commit was automatically rebased without conflicts.

[openjdk]

@DamonFool Pushed as commit 4a6da99.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.