Minor enhancements for performance investigation of Escape Analysis #16638
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change implements several minor clean-up items for Escape Analysis: 1) The top-level findIgnoreableUses method looks for calls to eaEscapeHelper and then calls a second method named findIgnoreableUses to mark uses of candidates for stack allocation as ignorable. However, that second method doesn't actually find ignoreable uses, it just marks them. This change renames that second method to markUsesAsIgnorable for clarity. Also corrected the spelling of 'ignorable.' 2) Most trace code in Escape Analysis identifies a candidate for stack allocation by the address of the TR::Node for the allocation, which is stored in the _node field of a Candidate object. In two cases, tracing was printing the address of the Candidate object itself. 3) Removed check of environment variable that is used to disable checking in loops for aliases of candidates for stack allocation, and removed method checkAllNewsOnRHSInLoop that was used in that case. The code that it allows to be disabled has been in place for about three years now, so it seems relatively safe. Signed-off-by: Henry Zongaro <zongaro@ca.ibm.com>
Added dynamic debug counters to gauge the number of contiguous and non-contiguous stack allocation opportunities that were found. Signed-off-by: Henry Zongaro <zongaro@ca.ibm.com>
Check the setting of the suppressEA option to decide whether a particular candidate for stack allocation should be prevented from being stack allocated. This can help test the effect on performance of individual opportunities for stack allocation. Signed-off-by: Henry Zongaro <zongaro@ca.ibm.com>
|
I looked at the changes and they look fine to me. Maybe the title of this PR ought to be changed to WIP until the OMR prereq is merged ? |
It's already a draft, which IMO is better than a WIP title prefix, at least because GitHub knows about it and actually blocks merging. I think we only ever had the WIP convention because draft PRs didn't exist yet. |
|
@vijaysun-omr Vijay, this is ready for review now that eclipse/omr#6872 has been promoted. |
|
Jenkins test sanity all jdk8,jdk11,jdk19 |
vijaysun-omr
approved these changes
Feb 3, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request contains two enhancements for investigation of the run time performance impact of Escape Analysis:
suppressEAoption to decide whether a particular candidate for stack allocation should be prevented from being stack allocated. This can help test the effect on performance of individual opportunities for stack allocation.There are also a number of minor clean-up items:
findIgnoreableUsesmethods, one of which finds uses that can be ignored, the other which marks them as ignorable. Renamed the second tomarkUsesAsIgnorablefor clarity. Also corrected the spelling of "ignorable."Candidateobject rather than the address of theTR::Nodeobject associated with theCandidate- that is, theCandidate::_nodefield.checkAllNewsOnRHSInLoopwhich was only used under control of an environment variable. After three years, it is probably safe to remove it.Depends on OMR pull request eclipse/omr#6872