[0.46.0] Change order of tests for inline Unsafe.{get|put}* calls #19660
Conversation
This file contains hidden or 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
If the J9Class for java/lang/Class is available to the compiler, the
IL it generates inline for Unsafe.{get|put}{int|long|Object}* tests
whether the Object being accessed is null (for native memory
references), has the low-order bit set for the specified offset
and whether its J9Class is that of java/lang/Class, in that order.
Those tests are trying to distinguish whether a static field is being
accessed, which requires different code to access the memory than
all the other cases.
For both instance fields and static fields, the Object will be a non-
null reference. However, the low-order bit of the offset will never
be set for instance fields. Therefore, what is likely the most common
usage would benefit from having the low-order offset bit tested first.
This commit changes the order of the tests accordingly for that case.
Signed-off-by: Henry Zongaro <[email protected]>
Various places in the inliner refer to the target TR::Block, TR::TreeTop or TR::Node of a branch in the IL as the "if" path and the fall-through path as the "else" path. Programmers might have different intuitions about whether the fall-through path is the "if path" or the "else" path. To avoid confusion, this change updates variable names to make it clear whether they represent things that will be executed if the branch is taken or executed if the branch is not taken. Signed-off-by: Henry Zongaro <[email protected]>
|
@vijaysun-omr, may I ask you to consider this for the v0.46.0-release branch? |
|
I have reviewed that the code changes are what was merged in the head stream a couple of days back. Henry has done extensive testing that is mentioned in the head stream PR. |
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.
If the J9Class for
java/lang/Classis available to the compiler, the IL it generates inline forUnsafe.{get|put}{int|long|Object}*tests whether the Object being accessed is null (for native memory references), has the low-order bit set for the specified offset and whether its J9Class is that ofjava/lang/Class, in that order. Those tests are trying to distinguish whether a static field is being accessed, which requires different code to access the memory than all the other cases.For both instance fields and static fields, the Object will be a non-null reference. However, the low-order bit of the offset will never be set for instance fields. Therefore, what is likely the most common usage would benefit from having the low-order offset bit tested first.
This commit changes the order of the tests accordingly for that case.
This opportunity was identified by @vijaysun-omr and @ymanton.
This is a port of pull request #19640 to the v0.46.0-release branch.