Skip to content

Add Cache Line Writeback Instruction - #7253

Merged
0xdaryl merged 2 commits into
eclipse-omr:masterfrom
jmesyou:intrinsics/writeback0
Mar 6, 2024
Merged

Add Cache Line Writeback Instruction#7253
0xdaryl merged 2 commits into
eclipse-omr:masterfrom
jmesyou:intrinsics/writeback0

Conversation

@jmesyou

@jmesyou jmesyou commented Feb 7, 2024

Copy link
Copy Markdown
Contributor

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.

@BradleyWood BradleyWood left a comment

Copy link
Copy Markdown
Contributor

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?

@jmesyou
jmesyou force-pushed the intrinsics/writeback0 branch from 78f85d7 to 627e240 Compare February 22, 2024 16:18
@jmesyou
jmesyou force-pushed the intrinsics/writeback0 branch from 627e240 to a890bb5 Compare February 22, 2024 16:19
@jmesyou
jmesyou requested review from BradleyWood and removed request for charliegracie February 22, 2024 16:20
@jmesyou
jmesyou force-pushed the intrinsics/writeback0 branch from a890bb5 to 0602496 Compare February 26, 2024 22:37

@BradleyWood BradleyWood left a comment

Copy link
Copy Markdown
Contributor

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 force-pushed the intrinsics/writeback0 branch from 0602496 to ffa31a5 Compare March 5, 2024 20:48
@jmesyou
jmesyou requested a review from dsouzai as a code owner March 5, 2024 20:48
@jmesyou
jmesyou removed the request for review from dsouzai March 5, 2024 20:49
@jmesyou
jmesyou force-pushed the intrinsics/writeback0 branch from ffa31a5 to 7d4f945 Compare March 5, 2024 20:50
@jmesyou
jmesyou requested a review from BradleyWood March 5, 2024 20:52
@BradleyWood

Copy link
Copy Markdown
Contributor

jenkins build all

@jmesyou

jmesyou commented Mar 5, 2024

Copy link
Copy Markdown
Contributor Author

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

0xdaryl commented Mar 6, 2024

Copy link
Copy Markdown
Contributor

Jenkins build win

1 similar comment
@0xdaryl

0xdaryl commented Mar 6, 2024

Copy link
Copy Markdown
Contributor

Jenkins build win

@0xdaryl

0xdaryl commented Mar 6, 2024

Copy link
Copy Markdown
Contributor

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.

jmesyou added 2 commits March 6, 2024 21:32
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
jmesyou force-pushed the intrinsics/writeback0 branch from 7d4f945 to 2c3ffab Compare March 6, 2024 21:33
@jmesyou

jmesyou commented Mar 6, 2024

Copy link
Copy Markdown
Contributor Author

Updated the commit message (only)

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.

3 participants