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

Disable vector shift and rotate opcodes #7232

Merged
merged 1 commit into from
Jan 19, 2024

Conversation

gita-omr
Copy link
Contributor

  • disable vector shift and rotate opcodes until they comply with the JVM Specification

- disable vector shift and rotate opcodes until they comply
  with the JVM Specification
@gita-omr
Copy link
Contributor Author

@BradleyWood @Akira1Saitoh please review

@Akira1Saitoh
Copy link
Contributor

Do we need to disable vector rotate too? I think that the problem is that we do not apply AND to the shift amount operand.

@gita-omr
Copy link
Contributor Author

gita-omr commented Jan 18, 2024

Do we need to disable vector rotate too? I think that the problem is that we do not apply AND to the shift amount operand.

I thought we better disable them too until we confirm the exact Java semantic for rotate opcodes as well. They are not a part of the JVM Spec so we need to check when and how they are produced, also create tests (if we don't have them already).

@knn-k
Copy link
Contributor

knn-k commented Jan 18, 2024

Jenkins build aarch64,amac,xlinux,osx

Copy link
Contributor

@knn-k knn-k left a comment

Choose a reason for hiding this comment

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

The AArch64 part looks good to me.

@knn-k
Copy link
Contributor

knn-k commented Jan 18, 2024

See #7181 for the x86-64 macOS failure with TestJitBuilderAPIGenerator.

@0xdaryl 0xdaryl self-assigned this Jan 18, 2024
@0xdaryl
Copy link
Contributor

0xdaryl commented Jan 18, 2024

Why isn't the solution to this problem to disable the opcode in a J9-specific version of getSupportsOpCodeForAutoSIMD() on the platforms where this is a problem?

I'm concerned with all the Java-specific motivation here. I don't think the OMR implementations of these opcodes need to bend exclusively to Java's requirements. Rather, if there are parts of the implementation that may have differing semantics based on the language environment then those can be specialized with front end queries. For example, introduce a frontend query to ask whether the shift value needs to be masked or not (and if so, to what value).

@BradleyWood
Copy link
Contributor

Why isn't the solution to this problem to disable the opcode in a J9-specific version of getSupportsOpCodeForAutoSIMD() on the platforms where this is a problem?

As it currently stands, OpenJ9 doesn't implement getSupportsOpCodeForAutoSIMD()

@0xdaryl
Copy link
Contributor

0xdaryl commented Jan 18, 2024

As it currently stands, OpenJ9 doesn't implement getSupportsOpCodeForAutoSIMD()

Right, but it could is my point, and make avoiding this OpenJ9-specific problem entirely an OpenJ9-specific solution. Unless I'm misunderstanding the power of that function.

Fixing it may involve introducing language-specific queries to the OMR vector implementations to ask whether masking is required or not.

Copy link
Contributor

@0xdaryl 0xdaryl left a comment

Choose a reason for hiding this comment

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

In theory, an OpenJ9-specific solution should be possible by OpenJ9 providing its own implementation of getSupportsOpCodeForAutoSIMD(). However, in practice this is hampered by the way this function is currently mplemented in OMR (it turns out that function is overloaded in the CodeGenerator class and it is not straightforward to resolve). Because this limitation is introduced by OMR and the fact that I am aware that there are no other consumers of this code other than OpenJ9 at this point, I'm willing to let this merge as-is.

Ideally, this function's implementation should be changed such that it doesn't rely on an overloaded function name.

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.

5 participants