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

Rename helpersNeedRelocation to needRelocationsForHelpers #7661

Merged
merged 1 commit into from
Nov 9, 2019
Merged

Rename helpersNeedRelocation to needRelocationsForHelpers #7661

merged 1 commit into from
Nov 9, 2019

Conversation

dchopra001
Copy link
Contributor

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

@dchopra001
Copy link
Contributor Author

@fjeremic @mpirvu Can I get a review please.

Also see eclipse-omr/omr#4454 for related PR.

mpirvu
mpirvu approved these changes Oct 31, 2019
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.

LGTM

@mpirvu mpirvu self-assigned this Oct 31, 2019
@mpirvu
Copy link
Contributor

mpirvu commented Oct 31, 2019

Jenkins test sanity xlinux,win,plinux jdk11

@fjeremic
Copy link
Contributor

Also see eclipse/omr#4454 for related PR.

This is a dependent PR. We need to override the OMR function via the extension mechanism for things to work. FYI @mpirvu this can't be merged before eclipse-omr/omr#4454 which has pending reviews.

@fjeremic fjeremic added the depends:omr Pull request is dependent on a corresponding change in OMR label Oct 31, 2019
@dchopra001
Copy link
Contributor Author

dchopra001 commented Oct 31, 2019 •
edited
Loading

Since this query was renamed in the eclipse-omr/omr#4454, I have renamed it here as well. This PR will need another review. @mpirvu @fjeremic

And as mentioned before, there is now a dependency between the two PRs.

@dchopra001 dchopra001 changed the title Add helpersNeedRelocation query in the codegen Rename helpersNeedRelocation to needRelocationsForHelpers Oct 31, 2019
@dchopra001 dchopra001 mentioned this pull request Oct 31, 2019
Closed
32 tasks
mpirvu
mpirvu approved these changes Oct 31, 2019
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.

LGTM

runtime/compiler/x/codegen/J9UnresolvedDataSnippet.cpp Outdated Show resolved Hide resolved
@dchopra001
Rename helpersNeedRelocation to needRelocationsForHelpers
309fedd 
This query is renamed so as to make it more consistent with similar
queries.

This commit also introduces this query into the CodeGenerator.

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

The travis failure is:

TR::AMD64RegImm64Instruction::addMetaDataForCodeAddress(uint8_t*)’:
5251../../../../../omr/compiler/x/codegen/X86BinaryEncoding.cpp:2627:22: error: ‘class TR_J9VMBase’ has no member named ‘helpersNeedRelocation’; did you mean ‘callTargetsNeedRelocations’?
5252    if (comp->fej9()->helpersNeedRelocation())

The reason is that this build didn't pick up the dependent omr changes.

@fjeremic
Copy link
Contributor

fjeremic commented Nov 4, 2019

The reason is that this build didn't pick up the dependent omr changes.
Yeah that is expected. I think the change has been propagated so it should work now. I've restarted the Travis build and kicked off a new Jenkins build.

Jenkins test sanity xlinux,win,plinux jdk11

@dchopra001
Copy link
Contributor Author

All tests passed.

@mpirvu
Copy link
Contributor

mpirvu commented Nov 9, 2019

Since all tests have passed I am going to merge this change.

@mpirvu mpirvu merged commit 58d4907 into eclipse-openj9:master Nov 9, 2019
@dchopra001 dchopra001 deleted the helperAddress branch July 13, 2021 13:58
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

@fjeremic fjeremic fjeremic approved these changes

Assignees

@mpirvu mpirvu

Labels
comp:jit 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.
3 participants
@dchopra001 @mpirvu @fjeremic