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

x86: check xcr0 in AOT compiles #21241

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

BradleyWood
Copy link
Member

Issue: #19408

@BradleyWood BradleyWood requested a review from dsouzai as a code owner March 1, 2025 02:40
Copy link
Contributor

@dsouzai dsouzai left a comment

Choose a reason for hiding this comment

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

J9::X86::CPU::detect pretty much looks like a copy of OMR::X86::CPU::detect; why not just call OMR::X86::CPU::detect (from J9::X86::CPU::detect) and update the OMR copy with any differences?

@BradleyWood
Copy link
Member Author

OMR::X86::CPU::detect returns only features enabled by OMR. If OpenJ9, tries to use extra features that OMR doesn't have enabled, then we can't just manually enabled these features in OpenJ9 if CPUID supports them. OpenJ9 doesn't know why a feature is or is/isn't enabled in OMR. It could be because OMR hasn't enabled it, or because the target CPU/OS doesn't support it. We end of back at the same spot, forced to check XCR0 again.

Your suggestion probably won't cause any issues unless someone tries to enabled a feature in OpenJ9 that OMR doesn't enable. Fixing this kind of thing would require changes in both OMR and OpenJ9, which I don't think there is time for at the moment. There are a few customers using windows server 2012 (which is technically out of support) that this affects.

@JamesKingdon
Copy link
Contributor

Hi Brad, thanks for fixing this. We have another customer case open at the moment for this problem and it looks like we might see more after WebSphere started shipping the recent JVM release.
@0xdaryl for info

@dsouzai
Copy link
Contributor

dsouzai commented Mar 3, 2025

At the risk of sounding unsympathetic, the original issue was opened almost a year ago and the last customer update was back in Sept, so I personally think we should fix this properly. However, I don't want to explicitly prevent this from going forward, so I'll defer this to @0xdaryl or @hzongaro.

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