Skip to content
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

Recognize unsafe unaligned getters/setters #19668

Merged
merged 2 commits into from
Nov 13, 2024

Conversation

nbhuiyan
Copy link
Member

@nbhuiyan nbhuiyan commented Jun 10, 2024

This PR re-introduces changes in #19420 and addresses the review comments received there.

Contrary to the discussion in #19420, I have not found any evidence that unaligned access is not permitted in Z only. Existing CG flag SupportsAlignedAccessOnly has been used for some versions of Power as well as aarch64 and arm32. Supporting unaligned access is something that most platforms are capable of, and therefore the default behaviour should be to transform the unaligned access operation methods, unless a back-end sets the SupportsAlignedAccessOnly flag.

@nbhuiyan nbhuiyan requested a review from dsouzai as a code owner June 10, 2024 20:53
@nbhuiyan
Copy link
Member Author

@jdmpapin Requesting review.

@nbhuiyan
Copy link
Member Author

@r30shah I'd appreciate your input on whether unaligned access is permitted on Z or not. If it is not permitted, was there a reason SupportsAlignedAccessOnly is not set during Z codegen initialization?

@r30shah
Copy link
Contributor

r30shah commented Jun 13, 2024

Memory references on Z are required to be aligned to operand size to ensure that all bytes within the operands are read as block concurrent as observed by other CPUs and channel programs. If the atomicity is not required - operand access can be done with unaligned address. So That is why I think code-gen on Z does not set SupportsAlignedAccessOnly.

Copy link
Contributor

@jdmpapin jdmpapin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just remembered some feedback I had originally sent directly to Jack. I had applied the changes to my working copy, and I was unable to see the transformation kick in. Here are a few pointers from when I was poking around looking at it

inlineUnsafeCall() is called only when the method satisfies isInlineableJNI()

if (isInlineableJNI(calleeSymbol->getResolvedMethod(),callNode))
{
if (performTransformation(comp(), "%sInlining jni %s into %s\n", OPT_DETAILS, calleeSymbol->signature(comp()->trMemory()), callerSymbol->signature(comp()->trMemory())))
{
if (calltarget->_myCallSite->isIndirectCall())
return true;
if (inlineGetClassAccessFlags(calleeSymbol, callerSymbol, callNodeTreeTop, callNode))
guard->_kind = TR_NoGuard;
else if (inlineUnsafeCall(calleeSymbol, callerSymbol, callNodeTreeTop, callNode))
guard->_kind = TR_NoGuard;
}
return true;
}

In isInlineableJNI(), here's where getInt(), etc., would be accepted:
if (method->convertToMethod()->isUnsafeWithObjectArg(comp) || method->convertToMethod()->isUnsafeCAS(comp))
{
// In Java9 sun/misc/Unsafe methods are simple Java wrappers to JNI
// methods in jdk.internal, and the enum values above match both. Only
// return true for the methods that are native.
if (!TR::Compiler->om.canGenerateArraylets() || (callNode && callNode->isUnsafeGetPutCASCallOnNonArray()))
return method->isNative();
else
return false;
}

Two problems:

  • isUnsafeWithObjectArg() doesn't know about the ...Unaligned() methods, so it won't return true
  • the method isn't native

So I think that's what's involved, but in any case please confirm that you can see the transformation happening before we move on

runtime/compiler/optimizer/InlinerTempForJ9.cpp Outdated Show resolved Hide resolved
runtime/compiler/optimizer/J9Inliner.hpp Outdated Show resolved Hide resolved
The commits adds support for unaligned getter/setters in the
Unsafe class for the JIT.

Unaligned getters and setters are treated the same as their
aligned equivalents for ISAs which permit unaligned reads/stores.
This requires a change to the inliner to consider whether
to emit an unsafe store/read based on whether the target ISA permits
unaligned accesses. So far only the Z target does not permit unaligned
accesses, so the getters/setters will fall back to their Java
software implementation.

Signed-off-by: James You <[email protected]>
Also-by: Nazim Bhuiyan <[email protected]>
For Unsafe_getXUnaligned methods, which are wrappers to
native methods that contain some runtime checks, we benefit from directly
inlining them in inlineUnsafeCall as if they were their underlying native
methods, if we can determine that it is safe to do so.

This commit introduces the changes necessary to correctly handle such
non-native unaligned getters and setters, and enables transforming them
to simple loads and stores based on whether the platform supports
aligned access only.

Signed-off-by: Nazim Bhuiyan <[email protected]>
@nbhuiyan
Copy link
Member Author

nbhuiyan commented Nov 6, 2024

@jdmpapin I have addressed your comments and have verified that the transformations are taking place with the current implementation.

@jdmpapin
Copy link
Contributor

Jenkins test sanity all jdk11,jdk21

@jdmpapin
Copy link
Contributor

The AArch64 Linux JDK11 sanity.openjdk failure is #13756

The Windows JDK21 sanity.openjdk failure is #18463

@jdmpapin
Copy link
Contributor

Jenkins compile xlinux jdk8

@jdmpapin jdmpapin merged commit 4b8e6ef into eclipse-openj9:master Nov 13, 2024
50 of 52 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants