Deprecate HPR support #3654
Merged
Deprecate HPR support #3654
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 commit deprecates GRA HPR support and cleans up the related APIs in the common GRA infrastructure. We also clean up the HPR support in the register pressure simulator which is surprisingly intertwined with GRA HPR support. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>
Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>
In addition fold: - setSpilledToHPR - isSpilledToHPR HPRs can no longer be spilled to (since they do not exist) so these APIs can be folded away as well as the corresponding flag. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>
This API was used to do a linear walk through the HPR register table and find the real (HPR) register which is assigned to a particular virtual register. This API is in hindsight pretty expensive given the number of places it is called in. We fold it away here and all the code that relies on it. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>
Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>
This list was used to keep track of blocked registers (HPR/GPR) that upgrades/spills etc. should not use. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>
The `findBestFreeRegister` API was aware of HPRs and no longer needs to be, so we remove the `needsHighWord` parameter as false and simplify the API. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>
Similarly to `needsHighWord` the `availHighWordRegMap` parameter used in several APIs is no longer needed as there are no HPRs available for assignment. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>
With the removal of `availHighWordRegMap` several of the default parameters of the `freeBestRegister` API are now "misplaced", or better put, not in the best order. In this commit we fix up the parameters to something more intuitive. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>
With HPRs almost gone out of the picture we can stop generating HPR register dependencies since nothing can be assigned to them. Note that the only piece we really need to keep is the KillVolHighRegs register dependency which is used to evict volatile HPRs on 32-bit linkage points. This commit will greatly improve the compile time of methods as we no longer have to traverse as many dependencies in quite a few local RA locations. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>
Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>
This is needlessly restrictive. The `registerExchange` API cannot handle register exchanges with one 32-bit and one 64-bit register and the code below guards against calling the `registerExchange` API in such situations. This can definitely be relaxed and the `registerExchange` API taught how to handle those cases. If you look at the places this API (`isAssignable`) is used you will see we effectively handle it there already. That code needs to be cleaned up and consolidated into the `registerExchange` API. I've left a TODO for this to be addressed at a later point. For now, simply disallow the aforementioned case. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>
Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>
This also effectively deprecates all HPR support and opens up opportunity to clean up and fold a lot of code throughout the codebase. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>
Now that all three HPR related transformations have been deprecated we can start cleaning up all HPR related APIs. We start with the `assignToHPR` API which was the primary interface between global and local RA. This API determined whether a particular virtual register is to be assigned to an HPR the first time the register becomes alive, or whether when generating an instruction using such a register needs to be upgraded to an equivalent HPR instruction. We fold this API to always return false and all the code that follows. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>
Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>
Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>
fjeremic
requested review from
0xdaryl,
charliegracie,
Leonardo2718,
mstoodle and
vijaysun-omr
as code owners
0xdaryl
requested changes
Mar 12, 2019
| @@ -3769,7 +3731,7 @@ TR_RegisterCandidates::computeAvailableRegisters(TR_RegisterCandidate *rc, int32 | |||
| while (bvi.hasMoreElements()) | |||
| { | |||
| int32_t reg = bvi.getNextElement(); | |||
| if (reg != parmReg) | |||
| if (reg != parmReg && (reg >= comp()->cg()->getFirstGlobalGPR() && reg <= comp()->cg()->getLastGlobalGPR())) | |||
There was a problem hiding this comment.
Can you provide some more details on the extra condition you added here?
There was a problem hiding this comment.
Looking back at my commits pre-interactive rebase I can see this line came from [1], which was the very first commit forming the basis for eclipse-openj9/openj9#4609 to add debug counters to keep track of various statistics.
There was a preexisting bug related to HPR GRA in which the mere presence of HPRs was invalidating all GPR linkage registers on Z to be considered for global allocation. This piece was code was part of a larger puzzle and was left and not cleaned up.
This is a mistake. Thanks for catching this. I've checked the the commit in [1] and this is the only such line which should not exist. I've removed it and fixed also the issue you noted about 58de15a which I forgot to squash as part of my commit cleanup before opening the PR.
I've also updated the copyright dates on all files I've changed. Fixed in d7f6331
[1] fjeremic@bf93d53#diff-b9e790fb0441d0bb55dbe69c6919695aR3772
Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>
This API really served two purposes: 1. Calculate the available register mask 2. Various HPR related checks We fold away the API and in the single place that it is used we replace with with just the caluclation of the available register map. In fact we leave a TODO here to pre-calculate this value as it should be a compile time constant for every method compilation. There should be no need to dynamically calculate it for every register dependency. The various HPR related checks set flags like `is64BitReg` and verified that there were no duplicate HPR/GPR register dependencies. Anecdotally this API actually consumed the most compile time in the entire compiler (not just the codegen, even the optimizer) because of the O(n^2) algorithm it used for each dependency list which can be rather large given GPRS, FPRs, VRFs, and HPRs. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>
Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>
Deprecate GCMap support for HPRs and all metadata stored for a particular method compilation. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>
During the refactoring and folding away of HPR related code we notice quite a bit of code duplication becomes apparent particularly in the local RA (OMRMachine.cpp). Because we do not need to special case for HPRs a lot of the code can actually be commoned up and various `is64BitReg` queries removed because the `if` and `else` paths are identical. Once the `is64BitReg` paths are folded away then it becomes apparent that the FPR and VRF paths are now identical to that of GPR and those can then be further folded away. Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>
fjeremic
force-pushed
the
improve-ra
branch
3 times, most recently
from
9cd374a to
86f9f19
Compare
Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>
|
@genie-omr build all |
0xdaryl
approved these changes
Mar 12, 2019
|
@genie-omr build xlinux |
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 PR deprecates all HPR support in the compiler. eclipse-openj9/openj9#4609 does a pretty good job of summarizing the various HPR support and how each piece plays a role in the overall picture. It also explains the shortcomings of the feature and why it is no longer worth the investment to keep it on life support.
Opening up the PRs early to get some testing going and to self-review some of the TODOs that were left as a result of the cleanup. The cleanup itself was rather straightforward. I will also collect performance data on how much compile time improvement this cleanup has yielded as disabling of the feature is not equivalent to this cleanup (I expect more than double compile time improvement as a result of this PR vs. just disabling the feature).
Issue: eclipse-openj9/openj9#4609
Fixes: #2739