-
Notifications
You must be signed in to change notification settings - Fork 707
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
AArch64: Enable Escape Analysis to allocate objects on stack #10264
AArch64: Enable Escape Analysis to allocate objects on stack #10264
Conversation
jenkins test sanity alinux64 jdk11 |
@@ -133,7 +133,7 @@ TR_EscapeAnalysis::TR_EscapeAnalysis(TR::OptimizationManager *manager) | |||
_dememoizationSymRef = NULL; | |||
|
|||
_createStackAllocations = true; | |||
_createLocalObjects = comp()->target().cpu.isX86() || comp()->target().cpu.isPower() || comp()->target().cpu.isZ(); | |||
_createLocalObjects = comp()->target().cpu.isX86() || comp()->target().cpu.isPower() || comp()->target().cpu.isZ() || comp()->target().cpu.isARM64(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this growing list of architecture checks (here and elsewhere in this PR) be replaced with a capability query instead? What are they really asking: whether a code generator is capable of mapping local objects to the stack? If so, I think it would be much cleaner to ask that question of a code generator and have each architecture answer true or false.
@vijaysun-omr or @andrewcraik any opinion or insight on whether that's what these architecture checks are really for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I agree with you, though the functionality being checked for is probably the entirety of supporting stack allocations, e.g. mapping local objects to the stack, emitting a stack alloc map etc. I don't see any reason why this cannot be a codegen query.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
@Akira1Saitoh , can you introduce a supportsStackAllocations()
query in the common code generator and have each architecture specialize it as necessary (basically, return true for every architecture other than ARM)?
You could probably do this in two commits: one to create the API, and the second to actually use it in place of the architecture checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I will add the API to the common code generator and replace those architecture checks with the query.
7720c4d
to
03f90eb
Compare
Pushed 2 commits to add |
jenkins test sanity alinux64 jdk11 |
Jenkins test sanity all jdk11 |
A full Jenkins run is required because multiple architectures are affected by this PR. |
This commit adds `supportsStackAllocations` query to the code generator. It is used to ask if the code generator supports stack allocations. aarch64/p/x/z code generators return true for this query. Signed-off-by: Akira Saitoh <saiaki@jp.ibm.com>
Replace architecture checks in Escape Analysis with the capability query which answers if the code generator supports stack allocations. Signed-off-by: Akira Saitoh <saiaki@jp.ibm.com>
03f90eb
to
298c91f
Compare
I fixed J9CodeGenerator.hpp for z and re-pushed commits. |
jenkins test sanity all jdk11 |
These commits enable Escape Analysis to allocate objects on stack for aarch64.
As in below conversation, we add
supportsStackAllocation
API to code generators. All code generators except ARM override it to return true. Architecture checks for supporting stack allocations in Escape Analysis are replaced with it.Signed-off-by: Akira Saitoh saiaki@jp.ibm.com