8275086: compiler/c2/irTests/TestPostParseCallDevirtualization.java fails when compiler1 is disabled

[sunny868]

Hi all,
Jtreg test case compiler/c2/irTests/TestPostParseCallDevirtualization.java fails for fastdebug mode on x86/aarch64/mips architecture when "--with-jvm-features=-compiler1" be used. the failed info is:

One or more @IR rules failed:

Failed IR Rules (1)
------------------
- Method "public int compiler.c2.irTests.TestPostParseCallDevirtualization.testMethodHandleCallWithCCP() throws java.lang.Throwable":
  * @IR rule 1: "@compiler.lib.ir_framework.IR(failOn={"#PRE#(\\\\d+(\\\\s){2}(CallStaticJava.*)+(\\\\s){2}===.*#IS_REPLACED#)", "invokeBasic"}, applyIf={}, applyIfAnd={}, applyIfOr={}, counts={"#PRE#(\\\\d+(\\\\s){2}(CallStaticJava.*)+(\\\\s){2}===.*#IS_REPLACED#)", "invokeStatic", "= 1"}, applyIfNot={})" 
    - failOn: Graph contains forbidden nodes:
        Regex 1: (\\d+(\\s){2}(CallStaticJava.*)+(\\s){2}===.*invokeBasic)
        Matched forbidden node:
          280  CallStaticJava  ===  5  6  7  8  1 ( 188  1  1  1  1  1  1 ) [[ 281  282  283  285 ]] # Static  java.lang.invoke.MethodHandle::invokeBasic
    - counts: Graph contains wrong number of nodes:
        Regex 1: (\\d+(\\s){2}(CallStaticJava.*)+(\\s){2}===.*invokeStatic)
        Expected 1 but found 0 nodes.

>>> Check stdout for compilation output of the failed methods

This is a patch to fix this problem. Please help review it.

Thanks,
Sun Guoyun

---

Progress

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

Issue

• JDK-8275086: compiler/c2/irTests/TestPostParseCallDevirtualization.java fails when compiler1 is disabled

Reviewers

• Igor Veresov (@veresov - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5903

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

Using diff file

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

[bridgekeeper]

👋 Welcome back sunny868! 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]

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

• hotspot-compiler

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

• 02: Full - Incremental (5c652578)
• 01: Full - Incremental (9ffdbeaf)
• 00: Full (ef103774)

[TobiHartmann]

Since this is a test for a C2 specific optimization, I would suggest to exclude the entire test if C2 is not available via a @requires vm.compiler2.enabled.

[sunny868]

In fact, I found this problem on the MIPS architecture. At present, we support C2 but not C1 on MIPS architecture.
And, if I change it to @requires vm.compiler1.enabled, the test still fails, and the jtreg prompt message is:

#Test results: no tests selected

[TobiHartmann]

Sorry, I misread your original message and thought the test fails if C2 is not available. So the issue is that the test fails if C1 is not available. I would assume this is due to missing profile information. Does increasing the number of warmup iterations via -DWarmup=10000 help?

[sunny868]

I found the compilation info with c1 is:

 937    b  3       compiler.c2.irTests.TestPostParseCallDevirtualization::testMethodHandleCallWithCCP (50 bytes)
 938    b  1       compiler.c2.irTests.TestPostParseCallDevirtualization::method2 (3 bytes)
 939    b  3       java.lang.reflect.Method::getParameterCount (6 bytes)
 940    b  3       java.lang.reflect.Method::invoke (65 bytes)          
 941    b  3       jdk.internal.reflect.DelegatingMethodAccessorImpl::invoke (10 bytes)
 942   !b  3       compiler.lib.ir_framework.test.CustomRunTest::invokeTest (76 bytes)
 943    b  3       jdk.test.lib.Asserts::assertEquals (7 bytes)         
 944   !b  3       jdk.internal.reflect.GeneratedMethodAccessor1::invoke (62 bytes)
 945    b  3       java.lang.invoke.LambdaForm$DMH/0x00000008000e3c00::invokeStatic (20 bytes)
 946    b  3       jdk.test.lib.Asserts::assertEquals (42 bytes)        
 947    b  4       compiler.c2.irTests.TestPostParseCallDevirtualization::testMethodHandleCallWithCCP (50 bytes)

here compile id 945 be called.
but the info without c1 is:

254     n       java.lang.invoke.MethodHandle::invokeBasic()I (native)   
255    b        java.lang.invoke.MethodHandleImpl::isCompileConstant (2 bytes)
256    b        compiler.c2.irTests.TestPostParseCallDevirtualization::method2 (3 bytes)
257    b        compiler.c2.irTests.TestPostParseCallDevirtualization::testMethodHandleCallWithCCP (50 bytes)

the compile id 254 be called

[sunny868]

In TestPostParseCallDevirtualization.java, use lable "applyIf=..." to disable testMethodHandleCallWithLoop and testMethodHandleCallWithCCP , other function such as testDynamicCallWithCCP testDynamicCallWithLoop can be tested normally.

[chhagedorn]

Though this issue is about excluding C1, I think the IR framework generally does not handle the case if C2 is excluded in the build (i.e. client VM). It only bails out of IR matching if C2 is excluded by command line flags. I will file a bug for it.

[TobiHartmann]

here compile id 945 be called.

What do you mean by that?

IR verification fails because post-parse call devirtualization was not able to replace the invokeBasic by an invokeStatic in the C2 compiled testMethodHandleCallWithCCP method. In theory, that should not be dependent on the availability of C1. So I'm wondering why it fails without C1. Did you check if increasing the number of warmup iterations helps?

[sunny868]

@TobiHartmann I don't quite understand the “post-parse call devirtualization" now. so I just find some info from -XX+PrintIdeal and -XX:+PrintComplation.

I have try again with -DWarmup=10000, and I set it in fileTestVMProcess.java, I`m not sure it is ok. the result is also FAILD.

Command Line:
/home/sunguoyun/jdk/build/linux-x86_64-server-fastdebug/jdk/bin/java -cp /home/sunguoyun/JTwork/classes/0/compiler/c2/irTests/TestPostParseCallDevirtualization.d:/home/sunguoyun/jdk/test/hotspot/jtreg/compiler/c2/irTests:/home/sunguoyun/JTwork/classes/0/test/lib:/home/sunguoyun/jdk/test/lib:/home/sunguoyun/JTwork/classes/0:/home/sunguoyun/jdk/test/hotspot/jtreg:/home/sunguoyun/jtreg/lib/javatest.jar:/home/sunguoyun/jtreg/lib/jtreg.jar -Djava.library.path=. -Xbootclasspath/a:. -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI -DWarmup=10000 -XX:+UnlockDiagnosticVMOptions -XX:-BackgroundCompilation -XX:+PrintCompilation -XX:-PrintOptoAssembly -Dir.framework.server.port=45519 -XX:-UseOnStackReplacement -XX:+PrintCompilation -XX:+UnlockDiagnosticVMOptions -XX:+LogCompilation -XX:CompileCommand=log,compiler.c2.irTests.TestPostParseCallDevirtualization::* -XX:CompileCommand=option,compiler.c2.irTests.TestPostParseCallDevirtualization::*,bool,PrintIdeal,true -XX:CompileCommand=option,compiler.c2.irTests.TestPostParseCallDevirtualization::*,bool,PrintOptoAssembly,true -XX:-OmitStackTraceInFastThrow -DShouldDoIRVerification=true -XX:-BackgroundCompilation -XX:CompileCommand=quiet -DWarmup=10000 compiler.lib.ir_framework.test.TestVM compiler.c2.irTests.TestPostParseCallDevirtualization

One or more @IR rules failed:

Failed IR Rules (1)
------------------
- Method "public int compiler.c2.irTests.TestPostParseCallDevirtualization.testMethodHandleCallWithCCP() throws java.lang.Throwable":
  * @IR rule 1: "@compiler.lib.ir_framework.IR(failOn={"#PRE#(\\\\d+(\\\\s){2}(CallStaticJava.*)+(\\\\s){2}===.*#IS_REPLACED# )", "invokeBasic"}, applyIf={}, applyIfAnd={}, applyIfOr={}, counts={"#PRE#(\\\\d+(\\\\s){2}(CallStaticJava.*)+(\\\\s){2}===.*#IS_REPLACED# )", "invokeStatic", "= 1"}, applyIfNot={})"
    - counts: Graph contains wrong number of nodes:
        Regex 1: (\\d+(\\s){2}(CallStaticJava.*)+(\\s){2}===.*invokeStatic )
        Expected 1 but found 0 nodes.

>>> Check stdout for compilation output of the failed methods

[sunny868]

I set -DWarmup=20000, test PASSED

[TobiHartmann]

The test also fails with -XX:-TieredCompilation and increasing the warmup does not seem to help. We need to investigate why post-parse call devirtualization does not work in that case.

[sunny868]

Maybe the FAIL caused by call_site_count = -1, after the code is modified as follows, then test case passed

--- a/src/hotspot/share/opto/callGenerator.cpp
+++ b/src/hotspot/share/opto/callGenerator.cpp
@@ -1049,7 +1049,8 @@ CallGenerator* CallGenerator::for_method_handle_call(JVMState* jvms, ciMethod* c
   ciCallProfile profile = caller->call_profile_at_bci(bci);
   int call_site_count = caller->scale_count(profile.count());
 
-  if (IncrementalInlineMH && call_site_count > 0 &&
+  if (IncrementalInlineMH && /*call_site_count > 0 &&*/
       (input_not_const || !C->inlining_incrementally() || C->over_inlining_cutoff())) {
     return CallGenerator::for_mh_late_inline(caller, callee, input_not_const);
   } else {

[sunny868]

The following modifications are also OK,But I'm not sure it's a reasonable fix

diff --git a/src/hotspot/share/ci/ciMethod.cpp b/src/hotspot/share/ci/ciMethod.cpp
index 862824c5b72..ff2c8ecb8e8 100644
--- a/src/hotspot/share/ci/ciMethod.cpp
+++ b/src/hotspot/share/ci/ciMethod.cpp
@@ -458,7 +458,8 @@ int ciMethod::check_overflow(int c, Bytecodes::Code code) {
 ciCallProfile ciMethod::call_profile_at_bci(int bci) {
   ResourceMark rm;
   ciCallProfile result;
-  if (method_data() != NULL && method_data()->is_mature()) {
+ if (ensure_method_data()){
     ciProfileData* data = method_data()->bci_to_data(bci);
     if (data != NULL && data->is_CounterData()) {
       // Every profiled call site has a counter.

[TobiHartmann]

Thanks for the details! I think it would be good if @iwanowww could take a look. He implemented post-parse call devirtualization.

[vnkozlov]

May be @veresov should look too.

I see that CompilationPolicy::is_mature()
https://github.com/openjdk/jdk/blob/master/src/hotspot/share/compiler/compilationPolicy.cpp#L889
uses Tier4InvocationThreshold = 5000 and ProfileMaturityPercentage=20 values to determine maturity (invocations > 1000):
https://github.com/openjdk/jdk/blob/master/src/hotspot/share/compiler/compilationPolicy.cpp#L268

I am not familiar with how many iterations the test calls compiled method. Does it uses -Xbatch?

[veresov]

Sounds like there is no MDO?
It should create the MDO (and start profiling) in this configuration after running in the interpreter for Tier0ProfilingStartPercentage=33 of the

scale = Tier0ProfilingStartPercentage / 100.0;
(i >= Tier3InvocationThreshold * scale) || (i >= Tier3MinInvocationThreshold * scale && i + b >= Tier3CompileThreshold * scale);

[sunny868]

MDO already exists, but it is not loaded.The following modification is to use ensure_ method_ data() function ensures that ciMethodData:: load_data() can be called,then method_data()->is_mature() will be TRUE.

diff --git a/src/hotspot/share/ci/ciMethod.cpp b/src/hotspot/share/ci/ciMethod.cpp
index 862824c5b72..ff2c8ecb8e8 100644
--- a/src/hotspot/share/ci/ciMethod.cpp
+++ b/src/hotspot/share/ci/ciMethod.cpp
@@ -458,7 +458,8 @@ int ciMethod::check_overflow(int c, Bytecodes::Code code) {
 ciCallProfile ciMethod::call_profile_at_bci(int bci) {
   ResourceMark rm;
   ciCallProfile result;
-  if (method_data() != NULL && method_data()->is_mature()) {
+ if (ensure_method_data()){
     ciProfileData* data = method_data()->bci_to_data(bci);
     if (data != NULL && data->is_CounterData()) {
       // Every profiled call site has a counter.

[veresov]

ensure_method_data() creates an MDO if it doesn't exist. I think that's what's happening. Because ciMethod::method_data() would normally do the load and it clearly doesn't happen.

[sunny868]

Thank you for @veresov reply.

There are two ways to pass this use case test:

1. use applyIf = {"TieredCompilation", "true"} to prohibit the testing of testMethodHandleCallWithLoop() and testMethodHandleCallWithCCP().
2. use-XX:Tier4InvocationThreshold=300 to make sure MDO can be created.

Which way is more reasonable?

[veresov]

There is a more elegant way to tweak the thresholds - CompileThresholdScaling. It affects all the thresholds so it's more robust way to do it. Alternatively, perhaps the test could do more iterations?

[sunny868]

Hi all, Please review the new patch, Thanks. I set TieredCompilation=true or TieredCompilation=false && CompileThresholdScaling<0.14 to make testing to PASSED.

[veresov]

0.14 is oddly low. I think we need to get to the root of this and figure out why it doesn't create the MDO when it should. Try running the test with -XX:+PrintTieredEvents and grep for testMethodHandleCallWithCCP. See what's happening there. Look at the mdo counters that it prints. What are the total counters when does it starts profiling (the mdo counters start increasing)?

[sunny868]

@veresov Through the parameter -XX:+printtieredevents, it can be found that under normal circumstances, when the invocation count is 256 of method java.lang.invoke.LambdaForm$DMH::invokeStatic, it is compiled into C1(Level 3) and MDO is created. But when layered compilation is turned off (-XX:-TieredCompilation), the MDO is not created until the invocation count is 1664, which is too late.
So is it more reasonable to lower the value of Tier0ProfileingStartPercentage or Tier3InvocationThreshold?

[veresov]

I think that's a pretty normal start of profiling in this configuration. We don't want to start profiling too early because it will have an adverse effect on startup. Historically, before tiered the profiling would start after 3300 invocations + back branches taken. It feels like the IR testing framework needs to be adjusted so that it warms the tests up longer (at least in this configuration).

[veresov]

So, now 5000 warmup iterations help? You mentioned previously that it had to be 20000 ? What changed?

[sunny868]

Trying 20,000 before just makes the test case occasionally successful. The core problem is that the warm-up should be for function java.lang.invoke.LambdaForm$DMH::invokeStatic (that is mh2, ) but not testMethodHandleCallWithLoop or testMethodHandleCallWithCCP, so I have a new patch, I think this is a minimal change for this test, meantime modify the iterations of warm-ups to 5000, then the test succeeds every time.

If we consider adding new IR test cases later, it might make more sense to modify the ir framework.

[veresov]

@chhagedorn, how do you think this PR should proceed? Would you consider fixing the framework to increase the warmup for the C2-only configuration?

[chhagedorn]

I think for now, going with @Warmup(5000) seems reasonable. Some thoughts about the IR framework:

• Default warmup: I agree that with -XX:-TieredCompilation, the warmup seems to be too short. I think it would make sense to use another default warmup value for C2-only configs if IR matching would be performed. Setting @Warmup, however, should always override it. I'm not sure what a good default value should be. Any thoughts @veresov?
• On top of that, our CI currently only runs -XX:-TieredCompilation in combination with CompileThreshold which is not whitelisted. This means that IR matching is not performed in that case. That's the reason why we have not detected this bug here in our testing. I think the IR framework should be improved to just ignore any CompileThreshold flag settings and allow IR matching to be performed with -XX:-TieredCompilation in our CI.

If others agree, I will file RFEs for both of these.

[veresov]

@chhagedorn, ok, let's go with the current solution for now then.
As for the default warmup, I would probably expose CompilationPolicy::min_invocations() through the WB API and do the warmup based on that. There are a lot of ways different flags may affect the thresholds, I think we just need an authoritative API point to tell us the minimum number of invocations. I would also disable various feedback mechanisms in the policy to make it more deterministic. So, Tier4LoadFeedback=1000000, Tier3LoadFeedback=1000000, Tier3DelayOn=1000000, Tier0Delay=1000000, TieredCompileTaskTimeout=1000000; or we should add a single option to disable all adaptive features.

[openjdk]

⚠️ @sunny868 the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout 8275086
$ git commit -c user.name='Preferred Full Name' --allow-empty -m 'Update full name'
$ git push

[openjdk]

@sunny868 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:

8275086: compiler/c2/irTests/TestPostParseCallDevirtualization.java fails when compiler1 is disabled

Reviewed-by: iveresov

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 282 new commits pushed to the master branch:

• 2b02b6f: 8274942: AssertionError at jdk.compiler/com.sun.tools.javac.util.Assert.error(Assert.java:155)
• bb92fb0: 8274930: sun/tools/jps/TestJps.java can fail with long VM arguments string
• 6a04899: 8275840: Add test to java/nio/channels/Channels/TransferTo.java to test transfer sizes > 2GB
• 01105d6: 8276367: ProblemList vmTestbase/nsk/jvmti/RedefineClasses/StressRedefineWithoutBytecodeCorruption/TestDescription.java
• 8fc16f1: 8275729: Qualified method names in CodeHeap Analytics
• fa4ce82: 8276260: (se) Remove java/nio/channels/Selector/Wakeup.java from ProblemList (win)
• 495c828: 8276188: Clarify "default charset" descriptions in String class
• cd778f5: 8202667: java/awt/Debug/DumpOnKey/DumpOnKey.java times out on Windows
• b889f2a: 8276175: codestrings.validate_vm gtest still broken on ppc64 after JDK-8276046
• 5b4e398: 8275766: (tz) Update Timezone Data to 2021e
• ... and 272 more: https://git.openjdk.java.net/jdk/compare/1e3069593e6f56714e1ee557b70930c2749d820c...master

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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@TobiHartmann, @veresov) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[sunny868]

/integrate

[openjdk]

@sunny868
Your change (at version 5c65257) is now ready to be sponsored by a Committer.

[sunny868]

@veresov Could you please sponsor it for me?

[veresov]

/sponsor

[openjdk]

Going to push as commit 87b926e.
Since your change was applied there have been 282 commits pushed to the master branch:

• 2b02b6f: 8274942: AssertionError at jdk.compiler/com.sun.tools.javac.util.Assert.error(Assert.java:155)
• bb92fb0: 8274930: sun/tools/jps/TestJps.java can fail with long VM arguments string
• 6a04899: 8275840: Add test to java/nio/channels/Channels/TransferTo.java to test transfer sizes > 2GB
• 01105d6: 8276367: ProblemList vmTestbase/nsk/jvmti/RedefineClasses/StressRedefineWithoutBytecodeCorruption/TestDescription.java
• 8fc16f1: 8275729: Qualified method names in CodeHeap Analytics
• fa4ce82: 8276260: (se) Remove java/nio/channels/Selector/Wakeup.java from ProblemList (win)
• 495c828: 8276188: Clarify "default charset" descriptions in String class
• cd778f5: 8202667: java/awt/Debug/DumpOnKey/DumpOnKey.java times out on Windows
• b889f2a: 8276175: codestrings.validate_vm gtest still broken on ppc64 after JDK-8276046
• 5b4e398: 8275766: (tz) Update Timezone Data to 2021e
• ... and 272 more: https://git.openjdk.java.net/jdk/compare/1e3069593e6f56714e1ee557b70930c2749d820c...master

Your commit was automatically rebased without conflicts.

[openjdk]

@veresov @sunny868 Pushed as commit 87b926e.

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

[chhagedorn]

@chhagedorn, ok, let's go with the current solution for now then. As for the default warmup, I would probably expose CompilationPolicy::min_invocations() through the WB API and do the warmup based on that. There are a lot of ways different flags may affect the thresholds, I think we just need an authoritative API point to tell us the minimum number of invocations. I would also disable various feedback mechanisms in the policy to make it more deterministic. So, Tier4LoadFeedback=1000000, Tier3LoadFeedback=1000000, Tier3DelayOn=1000000, Tier0Delay=1000000, TieredCompileTaskTimeout=1000000; or we should add a single option to disable all adaptive features.

Exposing this method sounds indeed more robust than a custom special handling within the IR framework. I will also consider setting the adaptive flags for the test VM to make it more deterministic. Thanks for your input! I will file the RFE accordingly.