Enable value types escape analysis #17594

hzongaro · 2023-06-14T17:09:42Z

These changes enable support for stack allocation of instances of value type classes in escape analysis. The changes are staged through a sequence of commits to aid in reviewing:

Commit 16d4e5ec4ac4d9bc390ebb2c8fb2229de283abb2 adds support for contiguous stack allocation of instances of value type classes
Commit 075335a1a65d53146590de54071cd3a6c6c91e14 adds support for non-contiguous stack allocation of instances of value type classes
Commit c2951330f46ea107e6d401e1154d51d3bedfeeef adds support for cold block heapification of instances of value type classes
Commit 404cd86f5cd9c6c651c3eee86b608cf104c7df86 adds special "harmless" recognition of array element load operations through calls to helper or non-helper methods
Commit 453a61a077717d12b4463a984cb1e872b4db8c66 adds some unit tests of escape analysis of value type instances
Commit 9e727ec12d64b65b38afe223fec21ab4615810dc is a minor tracing improvement

This pull request is currently marked as a draft pull request as it depends on a downstream OMR pull request. However, I am not expecting to make any further changes to this pull request, except as might be required to address review comments, of course.

Depends: eclipse-omr/omr#7031

a7ehuo

Here is the first half of the review on the first three commits. Just some minor comments. I'll take a look at the next three commits tomorrow

runtime/compiler/optimizer/EscapeAnalysis.cpp

a7ehuo

Here is the second half of the review. Just some very minor comments

test/functional/Valhalla/src/org/openj9/test/lworld/NullRestrictedTypeOptTests.java

runtime/compiler/optimizer/EscapeAnalysis.cpp

hzongaro · 2023-06-26T15:26:55Z

Annabelle @a7ehuo, I have addressed your review comments. I split the changes into several distinct commits to make it easier to squash each set of changes into the original set of commits that they affected.

I'll mark this pull request as ready for review now that OMR pull request eclipse-omr/omr#7031 has been promoted into OpenJ9-OMR.

a7ehuo

LGTM. Thank you for making these updates Henry! Just two questions to help me understand the code and one very minor formatting comment in the test code

runtime/compiler/optimizer/EscapeAnalysis.cpp

test/functional/Valhalla/src/org/openj9/test/lworld/NullRestrictedTypeOptTests.java

runtime/compiler/optimizer/EscapeAnalysis.cpp

a7ehuo

LGTM

test/functional/Valhalla/src/org/openj9/test/lworld/NullRestrictedTypeOptTests.java

runtime/compiler/optimizer/EscapeAnalysis.cpp

vijaysun-omr · 2023-07-10T22:15:18Z

runtime/compiler/optimizer/EscapeAnalysis.cpp

+
+         // generate store to the field
+         const TR::TypeLayoutEntry& fieldEntry = typeLayout->entry(i - 1);
+         TR::SymbolReference* symref = comp()->getSymRefTab()->findOrFabricateShadowSymbol(valueClass,


What is the explanation behind creating this fabricated symref ?

Are you asking why this symref was created but never used? If so, it looks like that was a mistake on my part in attempting to organize the commits so that the full support for escape analysis of value types accumulated in a reasonably logical fashion. A later commit in this pull request that adds cold block heapification support for value types actually makes use of the symref in a call to candidate->findOrSetFieldInfo and stores it in field._symRef.

If you'd like, I can rework these commits so that this findOrFabricate call only appears in that later commit.

Or are you looking for additional comments describing the purpose of the call to findOrFabricateShadowSymbol?

It was the former I think, i.e. I was wondering why it was introduced in this commit. But you explained that as just an issue with organizing the commits.

Would you like me to rework those two commits so that the call to findOrFabricateShadowSymbol only appears in the commit where it's first needed?

vijaysun-omr · 2023-07-10T22:17:29Z

What is the status with respect to flattened instances or arrays ? i.e. if I have a flattened field or array element inside an object being considered for stack allocation, will stack allocation be allowed to go ahead ?

runtime/compiler/optimizer/EscapeAnalysis.cpp

hzongaro · 2023-07-13T18:49:02Z

What is the status with respect to flattened instances or arrays ? i.e. if I have a flattened field or array element inside an object being considered for stack allocation, will stack allocation be allowed to go ahead ?

If an object that's created with a TR::new instruction has a field of a null-restricted field that is flattened, it is still eligible for stack allocation, as the leaf fields can all be zero initialized. If it has a null-restricted field that is not flattened, however, as things stand escape analysis is not prepared to create the default instance for the field and will not stack allocate the object. The check of whether the class is zero initializable happens in performAnalysisOnce and you can find the implementation in J9ClassEnv.cpp.

Similarl an array whose component type is a primitive value type will be filtered out from consideration in performAnalysisOnce. That's currently more restrictive than the case of flattenable fields, where the stack allocation can go ahead if the field is flattened. For flattened arrays, the stack allocation does not currently proceed, despite the fact that the fields of the elements can all be zero initialized.

I can open follow on issues to deal with those cases or, if you'd prefer, I can work on those enhancements under this pull request.

vijaysun-omr · 2023-07-14T10:10:38Z

Thanks for the explanation around flattened fields. I am fine with (and probably prefer) working out those other cases as and when the need arises than being proactive at this stage and doing it all especially since we don't have a great sense of relative priority with other things that you could be working on.

I certainly don't want that extra handling though in this PR, since this PR has more than enough code being committed at once anyway.

vijaysun-omr · 2023-07-14T10:12:59Z

I can run tests again once you update the commit with the different diagnostic message you proposed above in response to one of my comments.

hzongaro · 2023-07-14T15:02:58Z

I can run tests again once you update the commit with the different diagnostic message you proposed above in response to one of my comments.

I've updated that diagnostic message. If you don't mind, I'd like to first squash the more recent commits from review comments before you go ahead with running tests.

vijaysun-omr · 2023-07-14T16:13:04Z

Sure, please let me know when it is ready

This change considers value type instances created by newvalue operations as candidates for stack allocation. Only contiguous allocations are performed by this change, with no support for cold block heapification. Stack allocation of newvalue operations can be disabled by setting the TR_DisableValueTypeEA environment variable. This change also fixes a bug where references to any objects through newvalue operations were not being treated as escapes. Signed-off-by: Henry Zongaro <zongaro@ca.ibm.com>

This change adds support for stack allocation of value type instances using separate temporaries for each field if possible, which is known as non-contiguous stack allocation. Signed-off-by: Henry Zongaro <zongaro@ca.ibm.com>

If a candidate for stack allocation is detected to escape only in cold blocks, Escape Analysis might still allow it to be stack allocated with compensatory code in cold blocks that will copy it to an object on the heap, known as cold block heapification. This change adds cold block heapification support for instances of value types. Signed-off-by: Henry Zongaro <zongaro@ca.ibm.com>

Large segments of the code in makeContiguousLocalAllocation and makeNonContiguousLocalAllocation that handle value types are identical or nearly so. This change moves that code into a new method, makeNewValueLocalAllocation, to process the fields of the value type candidate instance for stack allocation. Signed-off-by: Henry Zongaro <zongaro@ca.ibm.com>

Calls to jitLoadFlattenableArrayElement in a loop will not yield a candidate allocation from a prior iteration, as it must be something that has already escaped into an array. It should be considered harmless by checkAllNewsOnRHSInLoopWithAliasing. Signed-off-by: Henry Zongaro <zongaro@ca.ibm.com>

This change implements some tests that expose opportunities for Escape Analysis involving instances of value classes. The tests do not verify that escape analysis actually performs stack allocation in those cases, but do involve situations where problems could arise if stack allocation is not performed correctly. Signed-off-by: Henry Zongaro <zongaro@ca.ibm.com>

Added code to Candidate::print to print any flags that are set on candidates for stack allocation. These changes were modelled on similar code that prints flags in OMR's TR_Debug::nodePrintAllFlags(). Signed-off-by: Henry Zongaro <zongaro@ca.ibm.com>

hzongaro · 2023-07-14T18:16:26Z

Sure, please let me know when it is ready

Thanks, @vijaysun-omr - I've squashed the commits back down to a reasonable set.

vijaysun-omr · 2023-07-14T18:57:49Z

jenkins test sanity all jdk17

vijaysun-omr · 2023-07-15T08:18:56Z

Checks have passed. Merging.

hzongaro added comp:jit perf depends:omr Pull request is dependent on a corresponding change in OMR project:valhalla Used to track Project Valhalla related work labels Jun 14, 2023

hzongaro requested review from vijaysun-omr and a7ehuo June 14, 2023 17:09

vijaysun-omr self-assigned this Jun 14, 2023

a7ehuo reviewed Jun 15, 2023

View reviewed changes

a7ehuo reviewed Jun 16, 2023

View reviewed changes

hzongaro commented Jun 23, 2023

View reviewed changes

runtime/compiler/optimizer/EscapeAnalysis.cpp Show resolved Hide resolved

hzongaro marked this pull request as ready for review June 26, 2023 15:26

hzongaro mentioned this pull request Jun 26, 2023

Create utility function for querying size of address field #17663

Open

a7ehuo reviewed Jun 27, 2023

View reviewed changes

runtime/compiler/optimizer/EscapeAnalysis.cpp Show resolved Hide resolved

test/functional/Valhalla/src/org/openj9/test/lworld/NullRestrictedTypeOptTests.java Outdated Show resolved Hide resolved

runtime/compiler/optimizer/EscapeAnalysis.cpp Show resolved Hide resolved

a7ehuo approved these changes Jun 27, 2023

View reviewed changes

a7ehuo reviewed Jul 10, 2023

View reviewed changes

test/functional/Valhalla/src/org/openj9/test/lworld/NullRestrictedTypeOptTests.java Outdated Show resolved Hide resolved

vijaysun-omr reviewed Jul 10, 2023

View reviewed changes

runtime/compiler/optimizer/EscapeAnalysis.cpp Outdated Show resolved Hide resolved

vijaysun-omr reviewed Jul 10, 2023

View reviewed changes

runtime/compiler/optimizer/EscapeAnalysis.cpp Show resolved Hide resolved

hzongaro added 4 commits July 14, 2023 11:01

Prototype non-contiguous stack allocation for value types

02596a3

This change adds support for stack allocation of value type instances using separate temporaries for each field if possible, which is known as non-contiguous stack allocation. Signed-off-by: Henry Zongaro <zongaro@ca.ibm.com>

hzongaro added 3 commits July 14, 2023 11:02

hzongaro force-pushed the enable-value-types-escape-analysis branch from 0f1dc52 to bbc4e9b Compare July 14, 2023 18:15

vijaysun-omr approved these changes Jul 15, 2023

View reviewed changes

vijaysun-omr merged commit 2208a88 into eclipse-openj9:master Jul 15, 2023

hzongaro deleted the enable-value-types-escape-analysis branch July 17, 2023 12:58

mpirvu mentioned this pull request Aug 10, 2023

JITServer throughput regression #17930

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable value types escape analysis #17594

Enable value types escape analysis #17594

hzongaro commented Jun 14, 2023

a7ehuo left a comment

a7ehuo left a comment •

edited

Loading

hzongaro commented Jun 26, 2023

a7ehuo left a comment

a7ehuo left a comment

vijaysun-omr Jul 10, 2023

hzongaro Jul 13, 2023

vijaysun-omr Jul 14, 2023

hzongaro Jul 14, 2023

vijaysun-omr commented Jul 10, 2023

hzongaro commented Jul 13, 2023

vijaysun-omr commented Jul 14, 2023

vijaysun-omr commented Jul 14, 2023

hzongaro commented Jul 14, 2023

vijaysun-omr commented Jul 14, 2023

hzongaro commented Jul 14, 2023

vijaysun-omr commented Jul 14, 2023

vijaysun-omr commented Jul 15, 2023

Enable value types escape analysis #17594

Enable value types escape analysis #17594

Conversation

hzongaro commented Jun 14, 2023

a7ehuo left a comment

Choose a reason for hiding this comment

a7ehuo left a comment • edited Loading

Choose a reason for hiding this comment

hzongaro commented Jun 26, 2023

a7ehuo left a comment

Choose a reason for hiding this comment

a7ehuo left a comment

Choose a reason for hiding this comment

vijaysun-omr Jul 10, 2023

Choose a reason for hiding this comment

hzongaro Jul 13, 2023

Choose a reason for hiding this comment

vijaysun-omr Jul 14, 2023

Choose a reason for hiding this comment

hzongaro Jul 14, 2023

Choose a reason for hiding this comment

vijaysun-omr commented Jul 10, 2023

hzongaro commented Jul 13, 2023

vijaysun-omr commented Jul 14, 2023

vijaysun-omr commented Jul 14, 2023

hzongaro commented Jul 14, 2023

vijaysun-omr commented Jul 14, 2023

hzongaro commented Jul 14, 2023

vijaysun-omr commented Jul 14, 2023

vijaysun-omr commented Jul 15, 2023

a7ehuo left a comment •

edited

Loading