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

Get target JITServer's CPU before CG object is created #10474

Merged
merged 1 commit into from
Sep 2, 2020
Merged

Get target JITServer's CPU before CG object is created #10474

merged 1 commit into from
Sep 2, 2020

Conversation

dchopra001
Copy link
Contributor

This is needed so that the codegen is able to set the correct
flags when initializing.

Signed-off-by: Dhruv Chopra Dhruv.C.Chopra@ibm.com

@dchopra001
Copy link
Contributor Author

Depends on eclipse-omr/omr#5506

@dchopra001
Copy link
Contributor Author

Tagging @harryyu1994 @mpirvu @dsouzai for review.

harryyu1994
harryyu1994 reviewed Aug 27, 2020
View reviewed changes
Copy link
Contributor

@harryyu1994 harryyu1994 left a comment

Choose a reason for hiding this comment

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

LGTM. The only risk I see is if we call TR::Compilation() somewhere else and didn't follow this. If this is the only place we call it then we are good.

runtime/compiler/compile/J9Compilation.cpp Outdated
// In JITServer, we would like to use JITClient's processor info for the compilation
// The following code effectively replaces the cpu with client's cpu through the getProcessorDescription() that has JITServer support
if (self()->getPersistentInfo()->getRemoteCompilationMode() == JITServer::SERVER)
if (((TR_J9VMBase *)fe)->isAOT_DEPRECATED_DO_NOT_USE() && TR::Compiler->target.cpu.isX86())
Copy link
Contributor

Choose a reason for hiding this comment

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

if (((TR_J9VMBase *)fe)->isAOT_DEPRECATED_DO_NOT_USE() && TR::Compiler->target.cpu.isX86())
      _target = TR::Compiler->relocatableTarget;

I think this needs to be moved out as well (It looks like you already added code outside the constructor but didn't remove it from here)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry forgot about this! Fixed in e7c02f3.

@dchopra001
Copy link
Contributor Author

The only risk I see is if we call TR::Compilation() somewhere else and didn't follow this

I have tested this code with an internal build and didn't see any issues.

@dchopra001 dchopra001 mentioned this pull request Aug 27, 2020
Merged
dsouzai
dsouzai requested changes Aug 27, 2020
View reviewed changes
Copy link
Contributor

@dsouzai dsouzai left a comment

Choose a reason for hiding this comment

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

I think you need to change the constructor in TR::Compilation also, ie https://github.com/eclipse/openj9/blob/master/runtime/compiler/compile/Compilation.hpp

@dsouzai
Copy link
Contributor

dsouzai commented Aug 27, 2020

I think you need to change the constructor in TR::Compilation also, ie https://github.com/eclipse/openj9/blob/master/runtime/compiler/compile/Compilation.hpp

Sorry, it looks like you did update Compilation.hpp; I got mixed up with the OMR PR...

@dchopra001
Copy link
Contributor Author

Travis build failed due to an infra issue:

apt-get install failed
562The command "sudo -E apt-get -yq --no-install-suggests --no-install-recommends $(travis_apt_get_options) install autoconf ca-certificates ccache cpio file g++-7 gcc-7 git git-core libasound2-dev libcups2-dev libdwarf-dev libelf-dev libfreetype6-dev libnuma-dev libx11-dev libxext-dev libxrender-dev libxt-dev libxtst-dev libxrandr-dev make nasm pkg-config realpath ssh unzip wget zip clang-3.8 llvm-3.8 libclang-3.8-dev llvm-3.8-dev" failed and exited with 100 during .

@dchopra001
Get target JITServer's CPU before CG object is created
2b4a2d7 
This is needed so that the codegen is able to set the correct
flags when initializing.

Signed-off-by: Dhruv Chopra <Dhruv.C.Chopra@ibm.com>
@dsouzai dsouzai added comp:jit comp:jit:aot comp:jitserver Artifacts related to JIT-as-a-Service project bug labels Aug 27, 2020
@dsouzai
Copy link
Contributor

dsouzai commented Aug 27, 2020

jenkins test sanity xlinuxjit jdk11

@dsouzai
Copy link
Contributor

dsouzai commented Aug 27, 2020

jenkins test sanity xlinuxjit jdk11 depends eclipse-omr/omr#5506

@dsouzai
Copy link
Contributor

dsouzai commented Aug 27, 2020

Jenkins didn't seem to use the OMR PR for some reason :S

@keithc-ca
Copy link
Contributor

Looks like it worked just fine to me (according to https://ci.eclipse.org/openj9/job/Build_JDK11_x86-64_linux_jit_Personal/144/consoleFull):

15:28:14  HEAD is now at a97de83a7a3... Merge d0e0cc197f4c988c45604260352b7173ccdabb5c into 2f7db34f45d5825579b194ae192f2824a11912e3

d0e0cc197f4c is the head of the branch for omr#5506; 2f7db34f45d5 is where the master branch was at the time.

@dsouzai
Copy link
Contributor

dsouzai commented Aug 27, 2020

Oh weird, I guess when I looked at it, the PR was showing the result of https://ci.eclipse.org/openj9/job/Pipeline_Build_Test_JDK11_x86-64_linux_jit/143/

@dsouzai dsouzai added the depends:omr Pull request is dependent on a corresponding change in OMR label Aug 27, 2020
@dsouzai
Copy link
Contributor

dsouzai commented Aug 28, 2020

JITServer test passed; will kick off regular PR build testing

@dsouzai
Copy link
Contributor

dsouzai commented Aug 28, 2020

jenkins test sanity all jdk11

@dsouzai
Copy link
Contributor

dsouzai commented Aug 28, 2020

sigh...

jenkins test sanity all jdk11 depends eclipse-omr/omr#5506

@dsouzai
Copy link
Contributor

dsouzai commented Sep 1, 2020

OMR PR has been merged; will merge this once that change is accepted into the openj9 branch of openj9-omr

@dsouzai dsouzai self-assigned this Sep 1, 2020
@dsouzai
Copy link
Contributor

dsouzai commented Sep 2, 2020

OMR change has promoted to openj9-omr.

@dsouzai dsouzai merged commit d3b01bf into eclipse-openj9:master Sep 2, 2020
@dchopra001 dchopra001 mentioned this pull request Sep 28, 2020
Closed
32 tasks
@dchopra001 dchopra001 deleted the cpuDetectionExt branch July 13, 2021 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@harryyu1994 harryyu1994 harryyu1994 left review comments

@mpirvu mpirvu mpirvu approved these changes

@dsouzai dsouzai dsouzai approved these changes

Assignees

@dsouzai dsouzai

Labels
bug comp:jit:aot comp:jit comp:jitserver Artifacts related to JIT-as-a-Service project depends:omr Pull request is dependent on a corresponding change in OMR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dchopra001 @dsouzai @keithc-ca @harryyu1994 @mpirvu