Implement LastITable for Z platform #21969
Conversation
|
Related changes on x86: #21939 |
ehsankianifar
force-pushed
the
Z_ImplementLastITable
branch
2 times, most recently
from
e8cd5a9 to
3b38250
Compare
ehsankianifar
force-pushed
the
Z_ImplementLastITable
branch
from
c5e326a to
653176f
Compare
ehsankianifar
force-pushed
the
Z_ImplementLastITable
branch
2 times, most recently
from
ab539cc to
a86f945
Compare
|
@r30shah thanks for review, when you have time, could you please take another look? |
ehsankianifar
force-pushed
the
Z_ImplementLastITable
branch
3 times, most recently
from
23da01a to
49ad59e
Compare
There was a problem hiding this comment.
@ehsankianifar Minor nitpicks.
One thing that I am bit concerned about and would like to confirm it is OK is with the way OOL was implemented here (With it's own ICF (No deps attached there), and multiple exit to diffrent location). Can I request you to collect a log file with traceRA with your unit test (4 Itable check case only) that we can inspect and confirm?
Also, just checking what do you think of making the test structure like following,
buildArgs (Already happens in mainline)
IPIC slot tests (In Mainline)
On Failure - Go To OOL
Snippet Call
...
iTable walk (Including last cached Itable)
...
9e4a871 to
89d1870
Compare
| TR_Debug * debugObj = cg->getDebug(); | ||
| TR::SymbolReference * methodSymRef = callNode->getSymbolReference(); | ||
|
|
||
| static bool enableLastITableCache = feGetEnv("TR_interfaceDispatchUsingLastITable") != NULL; |
There was a problem hiding this comment.
@ehsankianifar - A note, once the change is merged, please follow it up with another PR with some performance results (From Dacapo) to enable the last Cached ITable. If you want to do it as part of this PR I am ok as well. If you are chosing to follow it up with separate PR, can I request you to launch internal tests with Java 8 and Semeru 17/21 to confirm the functionality once it is enabled ?
| // If the iTableIterations is zero, then no instructions needed for iTable check. | ||
| bool checkITableEntries = iTableIterations > 0; | ||
| // If the iteration is larger than 0x7fff, check all entries without counting. | ||
| bool checkAllITableEnrties = iTableIterations > INT16_MAX; |
There was a problem hiding this comment.
Checking the getITableIterationsNumber - I do see why INT16_MAX is selected to enable walking of all ITable entries. Given that this is meant for tuning purpose only, I am OK, but I would have used two separate env option (i.e, TR_NumITableIterationsAfterLastITableCacheCheck and TR_CheckAllITableEntries - with exclusivity). Just a comment, no changes needed.
There was a problem hiding this comment.
Thanks, my current benchmark results show that we either get no benefit from iTable check or get the max benefit from checking all entries so maybe we decide to remove this number and make it no check or check all!
There was a problem hiding this comment.
Thanks @ehsankianifar - some minor nitpicks but overall changes are good. Once you confirm the question about how OOL code is structured (and make changes if needed) , please follow it up with the test request.
I would be OK to enable last cached ITable as part of this PR if you want to. In order to do that, while the PR testing runs, you need to provide some performance data atleast with Dacapo pmd here on the PR,
ehsankianifar
force-pushed
the
Z_ImplementLastITable
branch
3 times, most recently
from
6690c82 to
71aa600
Compare
|
Started sanity tests. |
There was a problem hiding this comment.
Thanks a lot @ehsankianifar. Changes are good, last couple recommendations. Please address/respond to it and launch internal build for testing (For SDK 8) on z/OS and Linux on Z.
ehsankianifar
force-pushed
the
Z_ImplementLastITable
branch
from
ed14d51 to
151b023
Compare
Check the lastITable and iTable before using interface lookup helper. First check the lastITable if it fails, check iTable entries. If any entry matches, calculate the entry point and jumps to the entry. If no entry matches, calls the helper method. Signed-off-by: Ehsan Kiani Far <[email protected]>
ehsankianifar
force-pushed
the
Z_ImplementLastITable
branch
from
ed9f9b7 to
abfdc3e
Compare
|
jenkins test sanity zlinux jdk8,jdk17,jdk21 |
|
All tests passed. Merging changes. |
Z jit code does not check the LastITable when an interface is called. This PR implements the Last I Table cache for Z platform.