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.

Sign up for GitHub

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

Jump to bottom

Store OMRProcessorDesc in AOT Header #9826

Merged
merged 1 commit into from
Jun 11, 2020
Merged

Store OMRProcessorDesc in AOT Header #9826

merged 1 commit into from
Jun 11, 2020

Conversation

harryyu1994
Copy link
Contributor

@harryyu1994 harryyu1994 commented Jun 8, 2020 •
edited
Loading

Previously we have TR_Processor and TR_ProcessorFeatureFlags in
AOT header. We will no longer be using TR_Processor and
TR_ProcessorFeatureFlags soon.

Issue: #9833
depends on: eclipse-omr/omr#5197

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

@harryyu1994 harryyu1994 force-pushed the newProcessorType branch 4 times, most recently from faf36c2 to 9bcacde Compare June 8, 2020 21:38
@harryyu1994 harryyu1994 changed the title Switch from TR_Processor to OMRProcessorArchitecture in AOT Header Store OMRProcessorDesc in AOT Header Jun 8, 2020
@harryyu1994 harryyu1994 force-pushed the newProcessorType branch 2 times, most recently from 373026e to a74352c Compare June 8, 2020 22:08
@harryyu1994
Copy link
Contributor Author

harryyu1994 commented Jun 8, 2020 •
edited
Loading

FYI @mpirvu

I just realized that the portable AOT work also benefits JITServer.
In JITServer our current implementation did not do what it's intended to do --> which is to have a per-compilation target processor. We did try to grab client's processor information and cache on the server but the implementation breaks if we have more than one client with different processors. The current implementation initializes the server-side processor information with the first client-side processor and it will carry on to use that one for its lifetime.

This item is a pre-requisite for #9731

This one depends on #9571 and eclipse-omr/omr#5197.

@harryyu1994 harryyu1994 force-pushed the newProcessorType branch 2 times, most recently from a507c53 to 3fca77a Compare June 9, 2020 15:37
@harryyu1994
Copy link
Contributor Author

@dsouzai Hi Irwin could you help review this one?

mpirvu
mpirvu reviewed Jun 10, 2020
View reviewed changes
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.

I have some inline comments/questions

runtime/compiler/aarch64/env/J9CPU.cpp Show resolved Hide resolved
runtime/compiler/compile/J9Compilation.cpp Outdated Show resolved Hide resolved
runtime/compiler/x/env/J9CPU.cpp Show resolved Hide resolved
@harryyu1994 harryyu1994 force-pushed the newProcessorType branch 2 times, most recently from 2c61276 to a72cd65 Compare June 10, 2020 19:41
@harryyu1994 harryyu1994 marked this pull request as ready for review June 10, 2020 20:11
@harryyu1994 harryyu1994 requested a review from mpirvu June 10, 2020 20:11
@mpirvu
Copy link
Contributor

mpirvu commented Jun 10, 2020

jenkins test sanity all jdk11

@mpirvu
Copy link
Contributor

mpirvu commented Jun 10, 2020

PPC compilation failures:

17:18:28  /home/jenkins/workspace/Build_JDK11_ppc64le_linux_Personal/build/linux-ppc64le-normal-server-release/vm/compiler/../compiler/p/env/J9CPU.cpp: In member function ‘OMRProcessorDesc J9::Power::CPU::getProcessorDescription()’:
17:18:28  /home/jenkins/workspace/Build_JDK11_ppc64le_linux_Personal/build/linux-ppc64le-normal-server-release/vm/compiler/../compiler/p/env/J9CPU.cpp:132:26: error: ‘TR::CompilationInfo’ has not been declared
17:18:28      if (auto stream = TR::CompilationInfo::getStream())
17:18:28                            ^~~~~~~~~~~~~~~
17:18:28  /home/jenkins/workspace/Build_JDK11_ppc64le_linux_Personal/build/linux-ppc64le-normal-server-release/vm/compiler/../compiler/p/env/J9CPU.cpp:134:26: error: ‘compInfoPT’ is not a member of ‘TR’
17:18:28         auto *vmInfo = TR::compInfoPT->getClientData()->getOrCacheVMInfo(stream);
17:18:28                            ^~~~~~~~~~
17:18:28  /home/jenkins/workspace/Build_JDK11_ppc64le_linux_Personal/build/linux-ppc64le-normal-server-release/vm/compiler/../compiler/build/rules/gnu/common.mk:81: recipe for target '/home/jenkins/workspace/Build_JDK11_ppc64le_linux_Personal/build/linux-ppc64le-normal-server-release/vm/compiler/../objs/compiler/p/env/J9CPU.o' failed

@harryyu1994
Store OMRProcessorDesc in AOT Header
900a08a 
Previously we have TR_Processor and TR_ProcessorFeatureFlags in
AOT header. We will no longer be using TR_Processor and
TR_ProcessorFeatureFlags soon.

Signed-off-by: Harry Yu <harryyu1994@gmail.com>
@harryyu1994
Copy link
Contributor Author

PPC compilation failures:

17:18:28  /home/jenkins/workspace/Build_JDK11_ppc64le_linux_Personal/build/linux-ppc64le-normal-server-release/vm/compiler/../compiler/p/env/J9CPU.cpp: In member function ‘OMRProcessorDesc J9::Power::CPU::getProcessorDescription()’:
17:18:28  /home/jenkins/workspace/Build_JDK11_ppc64le_linux_Personal/build/linux-ppc64le-normal-server-release/vm/compiler/../compiler/p/env/J9CPU.cpp:132:26: error: ‘TR::CompilationInfo’ has not been declared
17:18:28      if (auto stream = TR::CompilationInfo::getStream())
17:18:28                            ^~~~~~~~~~~~~~~
17:18:28  /home/jenkins/workspace/Build_JDK11_ppc64le_linux_Personal/build/linux-ppc64le-normal-server-release/vm/compiler/../compiler/p/env/J9CPU.cpp:134:26: error: ‘compInfoPT’ is not a member of ‘TR’
17:18:28         auto *vmInfo = TR::compInfoPT->getClientData()->getOrCacheVMInfo(stream);
17:18:28                            ^~~~~~~~~~
17:18:28  /home/jenkins/workspace/Build_JDK11_ppc64le_linux_Personal/build/linux-ppc64le-normal-server-release/vm/compiler/../compiler/build/rules/gnu/common.mk:81: recipe for target '/home/jenkins/workspace/Build_JDK11_ppc64le_linux_Personal/build/linux-ppc64le-normal-server-release/vm/compiler/../objs/compiler/p/env/J9CPU.o' failed

Added missing headers

@mpirvu mpirvu self-assigned this Jun 10, 2020
@mpirvu
Copy link
Contributor

mpirvu commented Jun 10, 2020

jenkins test sanity all jdk11

@mpirvu
Copy link
Contributor

mpirvu commented Jun 11, 2020

All tests passed.
@harryyu1994 If this fixes the JITServer compatibility problem we've seen recently, it needs to be double delivered in 0.21 release. This changeset is larger than what I am comfortable with to be ported to 0.21 release. Is there a small subset that will solve the JITServer compatibility?

@harryyu1994
Copy link
Contributor Author

All tests passed.
@harryyu1994 If this fixes the JITServer compatibility problem we've seen recently, it needs to be double delivered in 0.21 release. This changeset is larger than what I am comfortable with to be ported to 0.21 release. Is there a small subset that will solve the JITServer compatibility?

Unfortunately this is already the smallest possible change to have a per-client processor.

@mpirvu
Copy link
Contributor

mpirvu commented Jun 11, 2020

Does this change depend on any other changes that may not already be in 0.21 branch?

@mpirvu
Copy link
Contributor

mpirvu commented Jun 11, 2020

I guess the better approach here is to prepare these changes for 0.21 release: create a PR for 0.21 cherry-pick your changes on top of 0.21 branch and then thoroughly test it because jenkins testing for 0.21 does not work.
For now, I am going to merge this into master.

@mpirvu mpirvu merged commit a220f71 into eclipse-openj9:master Jun 11, 2020
@harryyu1994
Copy link
Contributor Author

Does this change depend on any other changes that may not already be in 0.21 branch?

eclipse-omr/omr#5197 needs to be in for the 0.21 release, would that be a huge problem for us?

@harryyu1994
Copy link
Contributor Author

harryyu1994 commented Jun 11, 2020 •
edited
Loading

Note that eclipse-omr/omr#5197 was tested extensively so I don't see it being a huge risk.

@mpirvu
Copy link
Contributor

mpirvu commented Jun 11, 2020

ok. It touches 17 files, that's what I am concerned about. I'll ask in the community channel

@harryyu1994 harryyu1994 mentioned this pull request Jun 11, 2020
Merged
@harryyu1994 harryyu1994 deleted the newProcessorType branch June 19, 2020 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpirvu mpirvu mpirvu approved these changes

Assignees

@mpirvu mpirvu

Labels
comp:jit:aot
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@harryyu1994 @mpirvu