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

Remove deprecated specialized epilogues #2066

Merged
merged 1 commit into from
Dec 5, 2017

Conversation

0xdaryl
Copy link
Contributor

@0xdaryl 0xdaryl commented Nov 28, 2017

  • Remove specialized epilogue code and fold surrounding code as necessary

Closes: #1738

Signed-off-by: Daryl Maier maier@ca.ibm.com

* Remove specialized epilogue code and fold surrounding code as necessary

Issue: eclipse-omr#1738

Signed-off-by: Daryl Maier <maier@ca.ibm.com>
@0xdaryl
Copy link
Contributor Author

0xdaryl commented Nov 28, 2017

I'd appreciate a review from @fjeremic and @ymanton please. This primarily impacts P and Z.

Copy link
Contributor

@fjeremic fjeremic left a comment

Choose a reason for hiding this comment

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

IBM Z changes LGTM.

@fjeremic
Copy link
Contributor

fjeremic commented Nov 28, 2017

Curious since it seems related, @0xdaryl are we keeping the shrink wrapping code around as well or is that slated for future deprecation? IIRC there were still functional issues with it and it never showed any real performance benefit.

@ymanton
Copy link
Contributor

ymanton commented Nov 28, 2017

P changes LGTM.

@vijaysun-omr vijaysun-omr self-assigned this Dec 1, 2017
@0xdaryl
Copy link
Contributor Author

0xdaryl commented Dec 4, 2017

@fjeremic , the shrink wrapping code was contributed primarily to demonstrate how one could do dataflow over TR::Instructions as we didn't want to lose that code. As an actual optimization, its worth was never really proven and it was never 100% functionally correct on all platforms.

Now that its part of the permanent git history of Eclipse OMR I'm fine with removing it. It will simplify some aspects of the CodeGenerator as well. The removal should be done as a single PR so that future historians can see all the changes that make up shrink wrapping conveniently should they need to.

@vijaysun-omr
Copy link
Contributor

vijaysun-omr commented Dec 5, 2017

I can only think of one (somewhat speculative) reason the shrink wrapping optimization should stay in the OMR codebase. The main reason why the optimization did not have the impact as originally envisaged was that the inliner became quite aggressive in Java and (generally speaking) has a high success rate for small/medium methods on hot paths. One question I cannot answer with certainty is : will the inliner be as successful in other languages that OMR is being considered for ? If it won't be as successful in some other language due to constraints unique to that scenario, then it might be more important to optimize prologue/epilogue costs (that shrink wrapping type optimization would be a part of).

@0xdaryl
Copy link
Contributor Author

0xdaryl commented Dec 5, 2017

I don't have any evidence to say whether it would be more beneficial in an inlining-constrained environment or not. The inliner technology in OMR is somewhat immature (the bulk of the smarts live in OpenJ9 and are highly tuned for Java) which means it may not be as effective or aggressive as Java. This may expose more opportunities @vijaysun-omr postulates. I think the only way we'll know for sure is to dust the code off and make it available for non-Java runtimes. I think the code as-written is reasonably general, but as we've seen with most pieces of code derived with a Java origin there will be some Java-specific semantics that will need to be generalized. Also, I do recall there were functional issues on at least Power that need investigation.

I think the shrink wrapping discussion should move to a separate issue (I'll create one). The fate of shrink wrapping in OMR is independent of this PR.

@vijaysun-omr
Copy link
Contributor

Fair enough, I will merge this PR independently of the broader shrink wrapping discussion.

@vijaysun-omr vijaysun-omr merged commit fae2007 into eclipse-omr:master Dec 5, 2017
@0xdaryl 0xdaryl deleted the specializedepilogues branch December 5, 2017 17:19
@0xdaryl 0xdaryl mentioned this pull request Dec 5, 2017
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