Skip to content
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

Replace CPU APIs on x86 #9421

Merged
merged 1 commit into from
Jun 2, 2020
Merged

Conversation

harryyu1994
Copy link
Contributor

@harryyu1994 harryyu1994 commented Apr 30, 2020

  • Replace old processor APIs with new APIs from the CPU class
  • Replace TR::Compiler target with per comp target

depends on: eclipse/omr#5151
depends on: eclipse/omr#5153

Signed-off-by: Harry Yu harryyu1994@gmail.com

@harryyu1994 harryyu1994 marked this pull request as ready for review April 30, 2020 22:21
@harryyu1994 harryyu1994 changed the title WIP: Replace old processor APIs with new APIs from the CPU class Replace old processor APIs with new APIs from the CPU class Apr 30, 2020
@harryyu1994 harryyu1994 force-pushed the replaceCPUAPI branch 2 times, most recently from 1d1984e to b1da9d6 Compare April 30, 2020 22:26
@harryyu1994 harryyu1994 changed the title Replace old processor APIs with new APIs from the CPU class Replace old x86 processor APIs with new APIs from the CPU class May 1, 2020
@harryyu1994 harryyu1994 force-pushed the replaceCPUAPI branch 4 times, most recently from f1b777d to b636af0 Compare May 2, 2020 02:05
@dsouzai
Copy link
Contributor

dsouzai commented May 4, 2020

I believe this needs to be merged in tandem with eclipse/omr#5151 right?

@harryyu1994
Copy link
Contributor Author

I believe this needs to be merged in tandem with eclipse/omr#5151 right?

Yes

@harryyu1994 harryyu1994 force-pushed the replaceCPUAPI branch 5 times, most recently from 58439ee to 126b6ac Compare May 19, 2020 19:09
@harryyu1994 harryyu1994 changed the title Replace old x86 processor APIs with new APIs from the CPU class Replace CPU API's on x86 May 19, 2020
@harryyu1994 harryyu1994 changed the title Replace CPU API's on x86 Replace CPU APIs on x86 May 19, 2020
Copy link
Contributor

@mpirvu mpirvu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throughout this commit I would replace all the asserts that will go away with TR_ASSERT_FATAL.

runtime/compiler/aarch64/env/J9CPU.cpp Outdated Show resolved Hide resolved
runtime/compiler/p/env/J9CPU.cpp Outdated Show resolved Hide resolved
runtime/compiler/x/codegen/AllocPrefetchSnippet.cpp Outdated Show resolved Hide resolved
runtime/compiler/x/codegen/J9CodeGenerator.cpp Outdated Show resolved Hide resolved
runtime/compiler/x/codegen/J9CodeGenerator.cpp Outdated Show resolved Hide resolved
@harryyu1994 harryyu1994 force-pushed the replaceCPUAPI branch 2 times, most recently from 3d8642e to 02f50d0 Compare May 21, 2020 17:28
@fjeremic fjeremic requested a review from mpirvu May 25, 2020 22:03
@fjeremic fjeremic self-assigned this May 25, 2020
Copy link
Contributor

@mpirvu mpirvu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fjeremic fjeremic self-requested a review May 25, 2020 22:46
@mpirvu
Copy link
Contributor

mpirvu commented May 25, 2020

TravisCI seems to have some problems:

[ 61%] Building CXX object runtime/omr/gc/CMakeFiles/omrgc.dir/base/GCCode.cpp.o
../../../../../openj9/runtime/compiler/env/J9CompilerEnv.cpp: In constructor ‘J9::CompilerEnv::CompilerEnv(J9JavaVM*, J9::RawAllocator, const J9::PersistentAllocatorKit&)’:
../../../../../openj9/runtime/compiler/env/J9CompilerEnv.cpp:32:13: error: no matching function for call to ‘OMR::CompilerEnv::CompilerEnv(J9::RawAllocator&, const J9::PersistentAllocatorKit&, OMRPortLibrary*)’
    javaVM(vm)
             ^
In file included from ../../../../../openj9/runtime/compiler/env/J9CompilerEnv.hpp:35:0,
                 from ../../../../../openj9/runtime/compiler/env/CompilerEnv.hpp:28,
                 from ../../../../../openj9/runtime/compiler/env/J9CompilerEnv.cpp:23:
../../../../../omr/compiler/env/OMRCompilerEnv.hpp:57:4: note: candidate: OMR::CompilerEnv::CompilerEnv(J9::RawAllocator, const J9::PersistentAllocatorKit&)
    CompilerEnv(TR::RawAllocator raw, const TR::PersistentAllocatorKit &persistentAllocatorKit);
    ^~~~~~~~~~~
../../../../../omr/compiler/env/OMRCompilerEnv.hpp:57:4: note:   candidate expects 2 arguments, 3 provided
../../../../../omr/compiler/env/OMRCompilerEnv.hpp:52:7: note: candidate: OMR::CompilerEnv::CompilerEnv(const OMR::CompilerEnv&)
 class OMR_EXTENSIBLE CompilerEnv
       ^~~~~~~~~~~
../../../../../omr/compiler/env/OMRCompilerEnv.hpp:52:7: note:   candidate expects 1 argument, 3 provided
../../../../../omr/compiler/env/OMRCompilerEnv.hpp:52:7: note: candidate: OMR::CompilerEnv::CompilerEnv(OMR::CompilerEnv&&)
../../../../../omr/compiler/env/OMRCompilerEnv.hpp:52:7: note:   candidate expects 1 argument, 3 provided
runtime/compiler/CMakeFiles/j9jit.dir/build.make:636: recipe for target 

@harryyu1994
Copy link
Contributor Author

harryyu1994 commented May 26, 2020

TravisCI seems to have some problems:

This is because this change depends on eclipse/omr#5151. It's expected.

@pshipton
Copy link
Member

It would be nice if this could be merged without breaking the OpenJ9 builds while waiting for OMR promotion. If the OMR changes can be merged first and OpenJ9 still builds, then nothing else is required. Otherwise, the OMR change should be merged first, and then an OMR promotion PR build can be run like "test sanity jdk8,jdk11 depends eclipse/omr#master". If this build passes or only has known failures, the tested OMR level can be pushed from master to the openj9 branch at the same time this PR is merged. Some preliminary testing should be done even before the OMR change is merged, as OMR promotion will be blocked waiting on the successful promotion build.

@harryyu1994
Copy link
Contributor Author

It would be nice if this could be merged without breaking the OpenJ9 builds while waiting for OMR promotion. If the OMR changes can be merged first and OpenJ9 still builds, then nothing else is required. Otherwise, the OMR change should be merged first, and then an OMR promotion PR build can be run like "test sanity jdk8,jdk11 depends eclipse/omr#master". If this build passes or only has known failures, the tested OMR level can be pushed from master to the openj9 branch at the same time this PR is merged. Some preliminary testing should be done even before the OMR change is merged, as OMR promotion will be blocked waiting on the successful promotion build.

The OMR changes will be merged first and they won't break OpenJ9.

@fjeremic
Copy link
Contributor

Jenkins test sanity xlinux,win jdk8

@harryyu1994
Copy link
Contributor Author

Jenkins test sanity xlinux,win jdk8

This change depends on eclipse/omr#5151 btw.

@fjeremic
Copy link
Contributor

Jenkins test sanity xlinux,win jdk8 depends eclipse/omr#5151

@harryyu1994
Copy link
Contributor Author

Seeing one failure on windows

14:01:50  FAILED test targets:
14:01:50  	cmdLineTester_SCHelperCompatibilityTests_win_1

Tried a test run without my change last night and saw this failure as well, therefore don't think it's caused by this change.

@fjeremic
Copy link
Contributor

That failure seems to be because of #9707.

@harryyu1994
Copy link
Contributor Author

Caught a zlinux crash with this one. Looking into it.

@fjeremic
Copy link
Contributor

Caught a zlinux crash with this one. Looking into it.

How is that possible if the code here only affects x86 codegen?

@harryyu1994
Copy link
Contributor Author

The problem is caused by this line in OMR: (This PR exposed this issue)

   if (processorDescription.processor >= OMR_PROCESSOR_S390_Z10 && TR::Options::getCmdLineOptions()->getOption(TR_DisableZ10))

Can't query options in detect() as options isn't initialized at that point. The fix for this is available here eclipse/omr#5153. So we should merge that PR before this.

@harryyu1994
Copy link
Contributor Author

harryyu1994 commented May 27, 2020

Caught a zlinux crash with this one. Looking into it.

How is that possible if the code here only affects x86 codegen?

OMR::Z::CPU::detect(OMRPortLibrary * const omrPortLib) is available in the codebase already.
Currently nothing is wrong because we haven't passed in an omrPortLib from OpenJ9.
This PR will pass in an omrPortLib and activate the code path in detect() on not just x86 but all the platforms.
It won't affect other platforms as long as we dont crash. (The other platforms are still using the old api and don't care what we do in detect() yet.)

TR::CPU
OMR::Z::CPU::detect(OMRPortLibrary * const omrPortLib)
   {
   if (omrPortLib == NULL)
      return TR::CPU();

   OMRPORT_ACCESS_FROM_OMRPORT(omrPortLib);
   OMRProcessorDesc processorDescription;
   omrsysinfo_get_processor_description(&processorDescription);

   if (processorDescription.processor >= OMR_PROCESSOR_S390_Z10 && TR::Options::getCmdLineOptions()->getOption(TR_DisableZ10))
      processorDescription.processor = OMR_PROCESSOR_S390_FIRST;

@harryyu1994 harryyu1994 changed the title Replace CPU APIs on x86 WIP: Replace CPU APIs on x86 May 28, 2020
@harryyu1994
Copy link
Contributor Author

May still affect other platforms. Let me verify, and possibly will try to limit it to x86 only.

@harryyu1994
Copy link
Contributor Author

harryyu1994 commented May 28, 2020

Actually all we need to do is to merge #9431 first. (I'm going to verify whether #9431 breaks anything).
Since the Z omr PR(eclipse/omr#5153) is merged, some of the supportFeatures() are already in the codebase. Though currently without an omrPortLib, they just redirect themselves to the old api's, but as soon as we pass in an omrPortLib, they will start using the new api's.
This PR will pass in an omrPortLib and effectively turn on the new api's and Z is not ready for that yet. On the other hand i think x86 is ready (verifying this now). This means we should merge the OpenJ9 Z PR first (#9431).
Z is not ready for the new api's because it still needs extra options checking (available in #9431) to get the new api's to behave the same way as the old ones.

- Replace old processor APIs with new APIs from the CPU class
- Replace TR::Compiler target with per comp target

depends on: eclipse/omr#5151

Signed-off-by: Harry Yu <harryyu1994@gmail.com>
@harryyu1994 harryyu1994 changed the title WIP: Replace CPU APIs on x86 Replace CPU APIs on x86 Jun 1, 2020
@harryyu1994
Copy link
Contributor Author

@fjeremic Hey Filip, I have verified that this one works on all platforms. I think it's ready to be merged.

@fjeremic
Copy link
Contributor

fjeremic commented Jun 2, 2020

Jenkins test sanity xlinux,win,osx jdk8,jdk11

@harryyu1994
Copy link
Contributor Author

Seeing 1 intermittent failure (Most likely not related to my change).
@fjeremic Could we retry the test?

Testing: jlm001
Test start time: 2020/06/02 13:13:38 Eastern Standard Time
Running command: "/home/jenkins/workspace/Test_openjdk11_j9_sanity.functional_x86-64_linux_Personal_testList_1/openjdkbinary/j2sdk-image/bin/java"  -Xgcpolicy:metronome -Xcompressedrefs  -Xdump    -agentlib:jvmtitest=test:jlm001 -cp "/home/jenkins/workspace/Test_openjdk11_j9_sanity.functional_x86-64_linux_Personal_testList_1/openjdk-tests/TKG/../../jvmtest/functional/cmdLineTests/jvmtitests/jvmtitest.jar" com.ibm.jvmti.tests.util.TestRunner
Time spent starting: 2 milliseconds
Time spent executing: 23379 milliseconds
Test result: FAILED
Output from test:
 [OUT] *** Testing [1/8]:	testCheckEnterCount
 [OUT] *** Test took 221 milliseconds
 [OUT] OK
 [OUT] 
 [OUT] *** Testing [2/8]:	testEnableJLM_START_INVALID
 [OUT] *** Test took 1 milliseconds
 [OUT] OK
 [OUT] 
 [OUT] *** Testing [3/8]:	testEnableJLM_START_STOP
 [OUT] *** Test took 0 milliseconds
 [OUT] OK
 [OUT] 
 [OUT] *** Testing [4/8]:	testEnableJLM_START_STOP_TIME_STAMP
 [OUT] *** Test took 1 milliseconds
 [OUT] OK
 [OUT] 
 [OUT] *** Testing [5/8]:	testGetJLMDUMP_TIME_STAMP
 [OUT] *** Test took 27 milliseconds
 [OUT] OK
 [OUT] 
 [OUT] *** Testing [6/8]:	testJLMDumpStats_DUMP_FORMAT_INVALID
 [OUT] *** Test took 1 milliseconds
 [OUT] OK
 [OUT] 
 [OUT] *** Testing [7/8]:	testNonRecursiveCount
 [OUT] *** Test took 104 milliseconds
 [OUT] OK
 [OUT] 
 [OUT] *** Testing [8/8]:	testSlowCountAndDumpStats
 [OUT] Slow count not as expected at least 5, was:4
 [OUT] *** Test took 22218 milliseconds
 [OUT] FAILED
 [OUT] 
>> Success condition was not found: [Return code: 0]

Testing: gmc001
Test start time: 2020/06/02 13:14:02 Eastern Standard Time
Running command: "/home/jenkins/workspace/Test_openjdk11_j9_sanity.functional_x86-64_linux_Personal_testList_1/openjdkbinary/j2sdk-image/bin/java"  -Xgcpolicy:metronome -Xcompressedrefs  -Xdump    -agentlib:jvmtitest=test:gmc001 -cp "/home/jenkins/workspace/Test_openjdk11_j9_sanity.functional_x86-64_linux_Personal_testList_1/openjdk-tests/TKG/../../jvmtest/functional/cmdLineTests/jvmtitests/jvmtitest.jar" com.ibm.jvmti.tests.util.TestRunner
Time spent starting: 3 milliseconds
Time spent executing: 776 milliseconds
Test result: PASSED

Testing: vgc001
Test start time: 2020/06/02 13:14:02 Eastern Standard Time
Running command: "/home/jenkins/workspace/Test_openjdk11_j9_sanity.functional_x86-64_linux_Personal_testList_1/openjdkbinary/j2sdk-image/bin/java"  -Xgcpolicy:metronome -Xcompressedrefs  -Xdump  -Xmx512m   -agentlib:jvmtitest=test:vgc001 -cp "/home/jenkins/workspace/Test_openjdk11_j9_sanity.functional_x86-64_linux_Personal_testList_1/openjdk-tests/TKG/../../jvmtest/functional/cmdLineTests/jvmtitests/jvmtitest.jar" com.ibm.jvmti.tests.util.TestRunner
Time spent starting: 2 milliseconds
Time spent executing: 923 milliseconds
Test result: PASSED

Test excluded: gjvmt001

Testing: gj9m001
Test start time: 2020/06/02 13:14:03 Eastern Standard Time
Running command: "/home/jenkins/workspace/Test_openjdk11_j9_sanity.functional_x86-64_linux_Personal_testList_1/openjdkbinary/j2sdk-image/bin/java"  -Xgcpolicy:metronome -Xcompressedrefs  -Xdump    -agentlib:jvmtitest=test:gj9m001 -cp "/home/jenkins/workspace/Test_openjdk11_j9_sanity.functional_x86-64_linux_Personal_testList_1/openjdk-tests/TKG/../../jvmtest/functional/cmdLineTests/jvmtitests/jvmtitest.jar" com.ibm.jvmti.tests.util.TestRunner
Time spent starting: 3 milliseconds
Time spent executing: 760 milliseconds
Test result: PASSED

Testing: Destroy shared class cache created by previous test
Test start time: 2020/06/02 13:14:04 Eastern Standard Time
Running command: "/home/jenkins/workspace/Test_openjdk11_j9_sanity.functional_x86-64_linux_Personal_testList_1/openjdkbinary/j2sdk-image/bin/java"  -Xgcpolicy:metronome -Xcompressedrefs  -Xdump    -Xshareclasses:destroyAll
Time spent starting: 2 milliseconds
Time spent executing: 461 milliseconds
Test result: PASSED

Testing: rbc001
Test start time: 2020/06/02 13:14:05 Eastern Standard Time
Running command: "/home/jenkins/workspace/Test_openjdk11_j9_sanity.functional_x86-64_linux_Personal_testList_1/openjdkbinary/j2sdk-image/bin/java"  -Xgcpolicy:metronome -Xcompressedrefs  -Xdump    -agentlib:jvmtitest=test:rbc001 -cp "/home/jenkins/workspace/Test_openjdk11_j9_sanity.functional_x86-64_linux_Personal_testList_1/openjdk-tests/TKG/../../jvmtest/functional/cmdLineTests/jvmtitests/jvmtitest.jar" com.ibm.jvmti.tests.util.TestRunner
Time spent starting: 3 milliseconds
Time spent executing: 1310 milliseconds
Test result: PASSED

Test excluded: nmr001

Testing: rrc001
Test start time: 2020/06/02 13:14:06 Eastern Standard Time
Running command: "/home/jenkins/workspace/Test_openjdk11_j9_sanity.functional_x86-64_linux_Personal_testList_1/openjdkbinary/j2sdk-image/bin/java"  -Xgcpolicy:metronome -Xcompressedrefs  -Xdump    -agentlib:jvmtitest=test:rrc001 -cp "/home/jenkins/workspace/Test_openjdk11_j9_sanity.functional_x86-64_linux_Personal_testList_1/openjdk-tests/TKG/../../jvmtest/functional/cmdLineTests/jvmtitests/jvmtitest.jar" com.ibm.jvmti.tests.util.TestRunner
Time spent starting: 2 milliseconds
Time spent executing: 1248 milliseconds
Test result: PASSED

Test Skipped: rrc001 : Hints specified for this test { HINT_SHARECLASSES } do not match with the hints for current mode { empty }

Test excluded: mt001

Test Skipped: Destroy shared class cache created by previous test : Hints specified for this test { HINT_SHARECLASSES } do not match with the hints for current mode { empty }

Testing: cma001
Test start time: 2020/06/02 13:14:07 Eastern Standard Time
Running command: "/home/jenkins/workspace/Test_openjdk11_j9_sanity.functional_x86-64_linux_Personal_testList_1/openjdkbinary/j2sdk-image/bin/java"  -Xgcpolicy:metronome -Xcompressedrefs  -Xdump    -XX:ForceClassfileAsIntermediateData -agentlib:jvmtitest=test:ria001,args:V3 -agentlib:jvmtitest=test:rca001,args:V4 -agentlib:jvmtitest=test:cma001 -cp "/home/jenkins/workspace/Test_openjdk11_j9_sanity.functional_x86-64_linux_Personal_testList_1/openjdk-tests/TKG/../../jvmtest/functional/cmdLineTests/jvmtitests/jvmtitest.jar" com.ibm.jvmti.tests.util.TestRunner
Time spent starting: 3 milliseconds
Time spent executing: 1003 milliseconds
Test result: PASSED

Test Skipped: cma001 : Hints specified for this test { HINT_SHARECLASSES } do not match with the hints for current mode { empty }

Test Skipped: Destroy shared class cache created by previous test : Hints specified for this test { HINT_SHARECLASSES } do not match with the hints for current mode { empty }

Testing: rnwr001
Test start time: 2020/06/02 13:14:08 Eastern Standard Time
Running command: "/home/jenkins/workspace/Test_openjdk11_j9_sanity.functional_x86-64_linux_Personal_testList_1/openjdkbinary/j2sdk-image/bin/java"  -Xgcpolicy:metronome -Xcompressedrefs  -Xdump    -agentlib:jvmtitest=test:rnwr001 -cp "/home/jenkins/workspace/Test_openjdk11_j9_sanity.functional_x86-64_linux_Personal_testList_1/openjdk-tests/TKG/../../jvmtest/functional/cmdLineTests/jvmtitests/jvmtitest.jar" com.ibm.jvmti.tests.util.TestRunner
Time spent starting: 2 milliseconds
Time spent executing: 1147 milliseconds
Test result: PASSED

Test Skipped: rnwr001 : Hints specified for this test { HINT_SHARECLASSES } do not match with the hints for current mode { empty }

Test Skipped: Destroy shared class cache created by previous test : Hints specified for this test { HINT_SHARECLASSES } do not match with the hints for current mode { empty }

Test Skipped: Cleanup any shared cache left behind : Hints specified for this test { HINT_SHARECLASSES } do not match with the hints for current mode { empty }

Testing: gsp001
Test start time: 2020/06/02 13:14:09 Eastern Standard Time
Running command: "/home/jenkins/workspace/Test_openjdk11_j9_sanity.functional_x86-64_linux_Personal_testList_1/openjdkbinary/j2sdk-image/bin/java"  -Xgcpolicy:metronome -Xcompressedrefs  -Xdump    -agentlib:jvmtitest=test:gsp001 -cp "/home/jenkins/workspace/Test_openjdk11_j9_sanity.functional_x86-64_linux_Personal_testList_1/openjdk-tests/TKG/../../jvmtest/functional/cmdLineTests/jvmtitests/jvmtitest.jar" com.ibm.jvmti.tests.util.TestRunner
Time spent starting: 2 milliseconds
Time spent executing: 734 milliseconds
Test result: PASSED

Testing: ee001
Test start time: 2020/06/02 13:14:10 Eastern Standard Time
Running command: "/home/jenkins/workspace/Test_openjdk11_j9_sanity.functional_x86-64_linux_Personal_testList_1/openjdkbinary/j2sdk-image/bin/java"  -Xgcpolicy:metronome -Xcompressedrefs  -Xdump    -agentlib:jvmtitest=test:ee001 -cp "/home/jenkins/workspace/Test_openjdk11_j9_sanity.functional_x86-64_linux_Personal_testList_1/openjdk-tests/TKG/../../jvmtest/functional/cmdLineTests/jvmtitests/jvmtitest.jar" com.ibm.jvmti.tests.util.TestRunner
Time spent starting: 2 milliseconds
Time spent executing: 672 milliseconds
Test result: PASSED


---TEST RESULTS---
Number of PASSED tests: 58 out of 59
Number of FAILED tests: 1 out of 59

---SUMMARY OF FAILED TESTS---
jlm001
-----------------------------


cmdLineTester_jvmtitests_5_FAILED

@fjeremic
Copy link
Contributor

fjeremic commented Jun 2, 2020

Jenkins test sanity xlinux jdk11

@fjeremic fjeremic merged commit 4165f1a into eclipse-openj9:master Jun 2, 2020
@harryyu1994 harryyu1994 deleted the replaceCPUAPI branch June 12, 2020 22:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants