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

Add Cache Line Writeback Instruction #7253

Merged
merged 2 commits into from
Mar 6, 2024

Conversation

jmesyou
Copy link
Contributor

@jmesyou jmesyou commented Feb 7, 2024

This commit adds the clwb instruction motivated
by the writeback0 intrinsic available in the Unsafe Java library.

The intrinsic permits users to write back a cache line if cache line writeback is enabled by the underlying VM. This instruction maps 1-1 with the intrinsic method.

The CPUID feature flag OMR_FEATURE_X86_CLWB is already implemented and should be used to check whether the instruction is supported in the underlying hardware.

Copy link
Member

@BradleyWood BradleyWood left a comment

Choose a reason for hiding this comment

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

I have a few questions and comments.

Can you remove "Fixes" from your PR and commit message?

To check for instruction support, OMR must enable the OMR_FEATURE_X86_CLWB feature. If OpenJ9 wishes to use this opcode, it must also do so.

Can you add some binary encoding test cases?

Copy link
Member

@BradleyWood BradleyWood left a comment

Choose a reason for hiding this comment

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

Have you tried calling supportsFeature(OMR_FEATURE_X86_CLWB)? I think supports_feature_test needs to know about it for now. I know its a bit annoying, but its required until we remove the old api.

@jmesyou jmesyou removed the request for review from dsouzai March 5, 2024 20:49
@jmesyou jmesyou requested a review from BradleyWood March 5, 2024 20:52
@BradleyWood
Copy link
Member

jenkins build all

@jmesyou
Copy link
Contributor Author

jmesyou commented Mar 5, 2024

Failures on windows are from infrastructure-related failures. MacOS tests failures are unrelated to this pull request, seem to be failing from #7181

@0xdaryl 0xdaryl self-assigned this Mar 6, 2024
@0xdaryl
Copy link
Contributor

0xdaryl commented Mar 6, 2024

Jenkins build win

1 similar comment
@0xdaryl
Copy link
Contributor

0xdaryl commented Mar 6, 2024

Jenkins build win

@0xdaryl
Copy link
Contributor

0xdaryl commented Mar 6, 2024

Minor nit: in your commit message, can you word wrap the line that begins "The intrinsic permits users to..."

We don't need to re-run CI if that's all you change. Thanks.

This commit adds the clwb instruction motivated
by the writeback0 intrinsic available in the Unsafe Java library.

The intrinsic permits users to write back a cache line
if cache line writeback is enabled by the underlying VM.
This instruction maps 1-1 with the intrinsic method.

The CPUID feature flag OMR_FEATURE_X86_CLWB is already implemented and
should be used to check whether the instruction is supported in the
underlying hardware. To use this instruction, the caller should check if
said feature flag is enabled for the cpu.

Signed-off-by: James You <james.you@protonmail.com>
@jmesyou
Copy link
Contributor Author

jmesyou commented Mar 6, 2024

Updated the commit message (only)

@0xdaryl 0xdaryl merged commit 107d389 into eclipse:master Mar 6, 2024
2 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants