Implement atomic add and swap codegen support for Z #2999

fjeremic · 2018-09-26T21:48:45Z

In support of #2958 we implement the existing intrinsics for Z. Another follow-up PR will address the new intrinsics which are proposed in #2174.

Implement Z codegen support for several atomic symbols
Document atomic symbol semantics
Document supportsAtomicAdd and introduce supportsAtomicSwap

Issue: #2958

Signed-off-by: Filip Jeremic [email protected]

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]>

Documents the semantics of the following codegen inlined intrinsics so that different codegens know exactly how to implement them: - atomicAdd32BitSymbol - atomicAdd64BitSymbol - atomicFetchAndAdd32BitSymbol - atomicFetchAndAdd64BitSymbol - atomicSwap32BitSymbol - atomicSwap64BitSymbol Signed-off-by: Filip Jeremic <[email protected]>

Signed-off-by: Filip Jeremic <[email protected]>

fjeremic · 2018-09-26T21:49:40Z

Requesting review from @liqunl @0dvictor for x86 code changes and verification of documentation, and @r30shah for Z codegen piece.

compiler/compile/OMRSymbolReferenceTable.hpp

compiler/codegen/OMRCodeGenerator.hpp

The width of the intrinsic to generate can be implied by the data type of the second child. Signed-off-by: Filip Jeremic <[email protected]>

This commit creates the following symbols: - atomicAddSymbol - atomicFetchAndAddSymbol - atomicSwapSymbol - atomicCompareAndSwapSymbol Because the corresponding "32Bit" and "64Bit" symbols are being used in the downstream OpenJ9 project a subsequent PR will remove them once the dependency is alleviated. The following symbols have been deprecated: - atomicAdd32BitSymbol - atomicAdd64BitSymbol - atomicCompareAndSwap32BitSymbol - atomicCompareAndSwap64BitSymbol in favor of the width-less versions. There are no known downstream projects using the above symbols so folding them is safe. Signed-off-by: Filip Jeremic <[email protected]>

Introduce a new API supportsNonHelper which determines whether inlining intrinsics is supported for a particular non-helper symbol. Since this API is more general we deprecate some existing APIs such as supportsAtomicAdd and supportsAtomicSwap. Signed-off-by: Filip Jeremic <[email protected]>

fjeremic · 2018-09-27T17:28:04Z

@0dvictor could you please review again? All comments have been addressed.

compiler/compile/OMRSymbolReferenceTable.hpp

0dvictor · 2018-09-27T17:52:19Z

Changes in common code and X86 CodeGen looks good to me.

liqunl · 2018-09-27T18:44:44Z

I like the idea of having the query supportsNonHelper. Changes in common and X86 look good to me. But will there be a follow-up PR to add the write barrier and read barrier for Object type?

fjeremic · 2018-09-27T18:49:36Z

But will there be a follow-up PR to add the write barrier and read barrier for Object type?

I'm not sure what you're referring to here. Could you elaborate what these are and where they should be added?

liqunl · 2018-09-27T19:06:07Z

Sorry, what I really wanted to ask is: does current implementation support object fields that are reference type? Reading from and writing to such fields needs read/write barrier.

fjeremic · 2018-09-27T19:33:44Z

Sorry, what I really wanted to ask is: does current implementation support object fields that are reference type? Reading from and writing to such fields needs read/write barrier.

No it doesn't. According to the documentation an address must be provided to these atomic symbols. Presumably whoever is providing the address has ensured that it is correct, i.e. if the address was calculated from a field load whoever is providing the address must ensure the read barrier was done.

Our evaluators for the atomic symbols are simple. The onus is on whoever generates them to ensure they follow the semantics according to the documentation.

compiler/z/codegen/OMRTreeEvaluator.hpp

Signed-off-by: Filip Jeremic <[email protected]>

fjeremic · 2018-09-28T15:41:24Z

All tagged reviews have approved. Off to @0xdaryl.

0dvictor

LGTM. (Sorry that I forgot to "officially" approve though I said LGTM in my comments.)

fjeremic · 2018-10-02T14:40:12Z

@0xdaryl / @vijaysun-omr all suggested reviewers have approved. Any concerns from committers side?

compiler/x/codegen/OMRTreeEvaluator.cpp

Signed-off-by: Filip Jeremic <[email protected]>

fjeremic · 2018-10-03T14:03:48Z

@0xdaryl any other concerns?

0xdaryl · 2018-10-03T21:15:32Z

@genie-omr build all

gita-omr

Sorry, for looking at this so late.

I find the name NonHelper a bit confusing. I would rather called it InlinedHelper.
@andrewcraik should not we do something special about aliasing for these new symbols?

0dvictor · 2018-11-12T17:05:05Z

I find the name NoHelper a bit confusing. I would rather called it InlineHelper.

There is an outstanding Issue (#3050) about renaming NonHelper. Though I believe everyone agreed to change its name, the final decision on its new name has not been made yet. The discussion was brought in one of complier architecture meetings. #3115

andrewcraik · 2018-11-12T17:12:37Z

I don't think any special aliasing is required because we default to a conservative default method aliasing which should be conservative enough to prevent unwanted code motion (https://github.com/eclipse/omr/blob/9cf9e1c660823c3f2ad333c9e6aca9c791581051/compiler/il/Aliases.cpp#L390).

gita-omr · 2018-11-12T17:15:22Z

Yes, I meant something more optimistic than the default aliasing. But I agree, it can be a separate feature.

fjeremic added 3 commits September 26, 2018 17:34

Document supportsAtomicAdd and introduce supportsAtomicSwap

f471392

Signed-off-by: Filip Jeremic <[email protected]>

fjeremic added the comp:compiler label Sep 26, 2018

fjeremic commented Sep 26, 2018

View reviewed changes

compiler/compile/OMRSymbolReferenceTable.hpp Outdated Show resolved Hide resolved

compiler/codegen/OMRCodeGenerator.hpp Outdated Show resolved Hide resolved

0dvictor reviewed Sep 26, 2018

View reviewed changes

compiler/codegen/OMRCodeGenerator.hpp Outdated Show resolved Hide resolved

fjeremic added 2 commits September 27, 2018 12:29

Fold atomicAdd32BitSymbol and atomicAdd32BitSymbol into atomicAddSymbol

5da969d

The width of the intrinsic to generate can be implied by the data type of the second child. Signed-off-by: Filip Jeremic <[email protected]>

fjeremic force-pushed the atomic-codegen-support branch from f7f9547 to 8d30aae Compare September 27, 2018 16:43

fjeremic force-pushed the atomic-codegen-support branch from 23dbc88 to a7704d1 Compare September 27, 2018 17:19

0dvictor reviewed Sep 27, 2018

View reviewed changes

compiler/compile/OMRSymbolReferenceTable.hpp Show resolved Hide resolved

r30shah reviewed Sep 27, 2018

View reviewed changes

compiler/z/codegen/OMRTreeEvaluator.hpp Show resolved Hide resolved

Add additional comments for atomic helpers on Z

9b514d3

Signed-off-by: Filip Jeremic <[email protected]>

r30shah approved these changes Sep 28, 2018

View reviewed changes

0dvictor approved these changes Sep 28, 2018

View reviewed changes

liqunl approved these changes Sep 28, 2018

View reviewed changes

0dvictor mentioned this pull request Oct 1, 2018

Add support for inlining new atomic methods on Power #3018

Merged

fjeremic mentioned this pull request Oct 2, 2018

Allow some Unsafe atomic ops to be inlined on POWER eclipse-openj9/openj9#3086

Merged

0xdaryl reviewed Oct 2, 2018

View reviewed changes

compiler/x/codegen/OMRTreeEvaluator.cpp Show resolved Hide resolved

Improve x86 code footprint for if/else ladder in directCallEvaluator

5d7999b

Signed-off-by: Filip Jeremic <[email protected]>

0xdaryl approved these changes Oct 3, 2018

View reviewed changes

0xdaryl self-assigned this Oct 3, 2018

0xdaryl merged commit 6301037 into eclipse-omr:master Oct 3, 2018

liqunl mentioned this pull request Oct 5, 2018

Optimize unsafe methods called from VarHandle eclipse-openj9/openj9#3140

Merged

gita-omr reviewed Nov 12, 2018

View reviewed changes

fjeremic mentioned this pull request Apr 18, 2019

Complete atomicCompareAndSwapReturn support on Z, Power, and AArch64 #3759

Open

fjeremic deleted the atomic-codegen-support branch February 26, 2021 19:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement atomic add and swap codegen support for Z #2999

Implement atomic add and swap codegen support for Z #2999

fjeremic commented Sep 26, 2018

fjeremic commented Sep 26, 2018

fjeremic commented Sep 27, 2018

0dvictor commented Sep 27, 2018

liqunl commented Sep 27, 2018

fjeremic commented Sep 27, 2018

liqunl commented Sep 27, 2018

fjeremic commented Sep 27, 2018

fjeremic commented Sep 28, 2018

0dvictor left a comment

fjeremic commented Oct 2, 2018

fjeremic commented Oct 3, 2018

0xdaryl commented Oct 3, 2018

gita-omr left a comment •

edited

Loading

0dvictor commented Nov 12, 2018

andrewcraik commented Nov 12, 2018

gita-omr commented Nov 12, 2018

Implement atomic add and swap codegen support for Z #2999

Implement atomic add and swap codegen support for Z #2999

Conversation

fjeremic commented Sep 26, 2018

fjeremic commented Sep 26, 2018

fjeremic commented Sep 27, 2018

0dvictor commented Sep 27, 2018

liqunl commented Sep 27, 2018

fjeremic commented Sep 27, 2018

liqunl commented Sep 27, 2018

fjeremic commented Sep 27, 2018

fjeremic commented Sep 28, 2018

0dvictor left a comment

Choose a reason for hiding this comment

fjeremic commented Oct 2, 2018

fjeremic commented Oct 3, 2018

0xdaryl commented Oct 3, 2018

gita-omr left a comment • edited Loading

Choose a reason for hiding this comment

0dvictor commented Nov 12, 2018

andrewcraik commented Nov 12, 2018

gita-omr commented Nov 12, 2018

gita-omr left a comment •

edited

Loading