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

Codegen Support for atomic method symbols #2958

Open
liqunl opened this issue Sep 12, 2018 · 7 comments
Open

Codegen Support for atomic method symbols #2958

liqunl opened this issue Sep 12, 2018 · 7 comments

Comments

@liqunl
Copy link
Contributor

liqunl commented Sep 12, 2018

x86 now supports atomic method symbols whose semantics are equivalent to common atomic operations such as fetch-and-add.

Currently we have the following method symbols

atomicAdd32BitSymbol
atomicAdd64BitSymbol
atomicFetchAndAdd32BitSymbol
atomicFetchAndAdd64BitSymbol
atomicSwap32BitSymbol
atomicSwap64BitSymbol

Four more symbols @0dvictor is trying to add on x86 are (see #2174)

atomicCompareAndSwap32BitSymbol
atomicCompareAndSwap64BitSymbol
atomicCompareAndSwapReturnValue32BitSymbol
atomicCompareAndSwapReturnValue64BitSymbol

They should be supported on all platforms. FYI @fjeremic @gita-omr @JamesKingdon

Edit:
Current implementation of atomicSwap* doesn't work on reference types because the lack of write bar and read bar. The support should be added.

Edit on October 5, 2018:
We also need to have separate symbols for object types. Object types are different because write barrier and read barrier might be required. They can be implemented the same as the primitive ones on OMR but having separate symbols allow the downstream projects to override the OMR implementation.

atomicSwapObject
atomicCompareAndSwapObject
atomicCompareAndSwapReturnValueObject
@fjeremic
Copy link
Contributor

Do we have tests added which exercise these symbols? How exactly do we know we've implemented them correctly?

@0dvictor
Copy link
Contributor

Do we have tests added which exercise these symbols? How exactly do we know we've implemented them correctly?

Unfortunately we don't have tests now. They are only being tested in OpenJ9 through OpenJ9's tests.

@fjeremic
Copy link
Contributor

Unfortunately we don't have tests now. They are only being tested in OpenJ9 through OpenJ9's tests.

I think we need to push the tests down into OMR then. If we wish to support an OMR API we should also have tests written against the respective API, otherwise the semantics of the APIs will diverge between codegens and non-OpenJ9 users of the APIs could potentially introduce changes which will break OpenJ9.

@liqunl
Copy link
Contributor Author

liqunl commented Sep 12, 2018

Not related to the discussion of adding test, but to show an idea of how large the item will be. Calls with the atomic method symbols share the same evaluator helper on X, which is https://github.com/eclipse/omr/blob/master/compiler/x/codegen/OMRTreeEvaluator.cpp#L2981

@zl-wang
Copy link
Contributor

zl-wang commented Sep 13, 2018

Although the existing evaluators for Unsafe.CAS or JUC's Atomic classes methods can be largely reused,
the question, as I can see it, needs to be clarified whether these method symbols carry Unsafe semantic implication of memory ordering (such that we put in expensive memory barriers, treating the underlying accessed memory as java volatiles). I hope they don't.

@liqunl
Copy link
Contributor Author

liqunl commented Sep 13, 2018

Although the existing evaluators for Unsafe.CAS or JUC's Atomic classes methods can be largely reused,
the question, as I can see it, needs to be clarified whether these method symbols carry Unsafe semantic implication of memory ordering (such that we put in expensive memory barriers, treating the underlying accessed memory as java volatiles). I hope they don't.

That raised a very interesting point. I was looking at this problem more from the Java side. I think we can or need to have different versions of the atomic symbols. C++ has acquire, release, weak and strong version for their atomics to enforce different memory orders. Java has also added variations to Unsafe methods since Java9 (the newly added methods call to the strong version so we do nothing about them currently). For the method symbols listed in this issue, I would say they are the strong ones, so we can reuse what we have for Java.

fjeremic added a commit to fjeremic/omr that referenced this issue Sep 26, 2018
Implement inlined intrinsic support for the following symbols as
defined in eclipse-omr#2958:

- atomicAdd32BitSymbol
- atomicAdd64BitSymbol
- atomicFetchAndAdd32BitSymbol
- atomicFetchAndAdd64BitSymbol
- atomicSwap32BitSymbol
- atomicSwap64BitSymbol

Signed-off-by: Filip Jeremic <[email protected]>
fjeremic added a commit to fjeremic/omr that referenced this issue Sep 26, 2018
Implement inlined intrinsic support for the following symbols as
defined in eclipse-omr#2958:

- atomicAdd32BitSymbol
- atomicAdd64BitSymbol
- atomicFetchAndAdd32BitSymbol
- atomicFetchAndAdd64BitSymbol
- atomicSwap32BitSymbol
- atomicSwap64BitSymbol

Signed-off-by: Filip Jeremic <[email protected]>
fjeremic added a commit to fjeremic/omr that referenced this issue Sep 27, 2018
Implement inlined intrinsic support for the following symbols as
defined in eclipse-omr#2958:

- atomicAdd32BitSymbol
- atomicAdd64BitSymbol
- atomicFetchAndAdd32BitSymbol
- atomicFetchAndAdd64BitSymbol
- atomicSwap32BitSymbol
- atomicSwap64BitSymbol

Signed-off-by: Filip Jeremic <[email protected]>
@liqunl
Copy link
Contributor Author

liqunl commented Oct 22, 2018

As code evolves, we're going to deprecate *[32Bit|64Bit]Symbol and use a general name for all data sizes. If we say we support nonhelper, we should implement it for all data sizes.
The following are all memory update intrinsics that should be supported.

atomicAddSymbol
atomicFetchAndAddSymbol
atomicSwapSymbol
atomicCompareAndSwapSymbol
atomicCompareAndSwapReturnValueSymbol

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants