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

Rework MemoryReference::accessStaticItem in Power for JITServer #4442

Merged
merged 3 commits into from
Dec 4, 2019

Conversation

dchopra001
Copy link
Contributor

@dchopra001 dchopra001 commented Oct 11, 2019

In this PR we make the changes needed to use front end queries instead of cg->compileRelocatableCode() when deciding whether to generate relocatable code inside accessStaticItem. This allows us to generate relocatable code for remote compilations in a consumer project such as OpenJ9, because the appropriate front-end query can return the right answer depending on the mode of compilation.

@dchopra001
Copy link
Contributor Author

Depends on #4441

@dchopra001
Copy link
Contributor Author

dchopra001 commented Oct 12, 2019

@mpirvu @ymanton

I wasn't able to use needRelocationsForStatics() for startPC and compiledMethod nodes on Power. This is the query used when making the decision on whether to relocate those relocation types on X and Z. However on Power this query won't always return the correct result. See 92f0951 for details.

In an earlier conversation, @ymanton had mentioned that perhaps the needRelocationsForBodyInfoData query might work better on Power. Although this will create inconsistency between Power and the other codegens. It might be better to create new queries for these data types as well. Also, isOutOfProcessCompilation() won't work in OMR, so we definitely can't use that query.

@dchopra001
Copy link
Contributor Author

This PR is part of the work in eclipse-openj9/openj9#6991

@dchopra001 dchopra001 mentioned this pull request Oct 16, 2019
6 tasks
@dchopra001 dchopra001 force-pushed the accessStaticItem branch 4 times, most recently from 10680c7 to de23b87 Compare November 22, 2019 19:20
@dchopra001 dchopra001 marked this pull request as ready for review November 22, 2019 19:21
Copy link
Contributor

@ymanton ymanton left a comment

Choose a reason for hiding this comment

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

I'm not 100% confident about these changes. Do you recall which of these changes are actually needed for JITServer and which might have been changed pro-actively because of existing checks for compileRelocatableCode()?

@dchopra001
Copy link
Contributor Author

I'm not 100% confident about these changes. Do you recall which of these changes are actually needed for JITServer and which might have been changed pro-actively because of existing checks for compileRelocatableCode()

As I spoke to you earlier, pretty much all of these changes are consistent with the checks done in x86. I saw failures related to most of these at the beginning of the Port, so I don't have failure details for all of them. However I'm pretty confident that most if not all of these changes are needed. After encountering every failure, whenever I reached this area of the code, I referred to the x86 equivalent version and tried to use the same query they were using. In some cases I couldn't use the same query, but we are in the process of addressing those right now.

@dchopra001
Copy link
Contributor Author

@ymanton There's still some work to be done here (mainly the following need a response from you: #4442 (comment), #4442 (comment)).

Other than that I think I've addressed everything. So I'll wait for another round of review from you now.

@dchopra001
Copy link
Contributor Author

dchopra001 commented Dec 4, 2019

@ymanton Ready for another review. I've done a rebase as well to resolve the merge conflicts.

This commit reorganizes this routine to broaden the scope of
when relocatable code can be generated. We now use front end
queries to determine when a relocatable instruction needs to
be generated. This is needed in cases where we are doing a remote
compile.

Signed-off-by: Dhruv Chopra <Dhruv.C.Chopra@ibm.com>
Signed-off-by: Dhruv Chopra <Dhruv.C.Chopra@ibm.com>
In some situations, this query was being used in addition
to another query that would also return the same answer. Since
it doesn't make sense to use this query anymore (as the other
one is more appropriate given the data type), this commit
removes uses of such queries.

Additionally, use of the new needRelocationsForCurrentMethodPC
query is added for symbol->isCompiledMethod data types.

Signed-off-by: Dhruv Chopra <Dhruv.C.Chopra@ibm.com>
Copy link
Contributor

@ymanton ymanton 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-assigned this Dec 4, 2019
@fjeremic
Copy link
Contributor

fjeremic commented Dec 4, 2019

@genie-omr build all

@dchopra001
Copy link
Contributor Author

dchopra001 commented Dec 4, 2019

X86 build failed with:

14:41:17  FAILED: /usr/lib/ccache/c++   -DJ9X86 -DLINUX -D_FILE_OFFSET_BITS=64 -D_X86_ -Iomr -I../omr/. -I../include_core -I. -I../util/hookable/. -I../thread/. -I../example/glue -I../util/hashtable/. -I../gc/base/vlhgc -I../gc/. -Igc -I../gc/base -I../gc/base/segregated -I../gc/base/standard -I../gc/include -I../gc/startup -I../gc/stats -I../gc/structs -I../gc/verbose -I../gc/verbose/handler_standard -pthread -fno-strict-aliasing -m32 -msse2 -std=c++0x -fPIC -MD -MT omr/startup/CMakeFiles/omrvmstartup.dir/omrvmstartup.cpp.o -MF omr/startup/CMakeFiles/omrvmstartup.dir/omrvmstartup.cpp.o.d -o omr/startup/CMakeFiles/omrvmstartup.dir/omrvmstartup.cpp.o -c /home/jenkins/workspace/PullRequest-linux_x86/omr/startup/omrvmstartup.cpp
14:41:17  c++: error: /home/jenkins/workspace/PullRequest-linux_x86/omr/startup/omrvmstartup.cpp: No such file or directory
14:41:17  c++: fatal error: no input files

Doesn't look related to this PR.

@fjeremic
Copy link
Contributor

fjeremic commented Dec 4, 2019

@genie-omr build x32linux

@fjeremic fjeremic merged commit 7189cd9 into eclipse-omr:master Dec 4, 2019
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.

4 participants