Skip to content

Add safeguard check for SIMD ops support #6558

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.

Sign up for GitHub

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

Jump to bottom
Merged
0xdaryl merged 1 commit into from
Jun 17, 2022
Merged

Add safeguard check for SIMD ops support #6558

0xdaryl merged 1 commit into from
Jun 17, 2022

Conversation

@r30shah
Copy link
Contributor

@r30shah r30shah commented Jun 9, 2022

This commit adds a check to getSupportsOpCodeForAutoSIMD to make sure
that SIMD opcode is supported only when we have support for Vector
instructions.

Signed-off-by: Rahil Shah rahil@ca.ibm.com

@r30shah r30shah marked this pull request as ready for review June 9, 2022 14:26
@r30shah r30shah requested a review from fjeremic as a code owner June 9, 2022 14:26
@r30shah
Copy link
Contributor Author

r30shah commented Jun 9, 2022

@joransiu Can I please get your review on these change? Slight background behind the change. The function getSupportsOpCodeForAutoSIMD is used to check if the opcode is supported on the platform providing more granular check. Historically I believe it was used by autoSIMD optimization which before performing optimization in general also checks if SIMD is enabled or not. As VectorAPI Expansion is also using getSupportsOpCodeForAutoSIMD we need to make sure it returns false on platform where SIMD support does not exist.

FYI @gita-omr

joransiu
joransiu approved these changes Jun 9, 2022
View reviewed changes
Copy link
Contributor

@joransiu joransiu left a comment

Choose a reason for hiding this comment

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

This change looks good, though setSupportsVectorRegisters() is only done in OpenJ9 right now. That logic should be migrated to OMR proper, so OMR based projects can benefit too.

@r30shah
Copy link
Contributor Author

r30shah commented Jun 10, 2022

@joransiu I agree. I am still trying to understand the reason behind initializing those flags in S390PrivateLinkage.cpp [1]. As I mentioned yesterday in the scrums, when I removed the setup of those flags from linkage and moved it to OMRCodeGenerator constructor for Z, I was hitting Assert in [2]. I need to do bit more investigations to understand the reasons behind that.

[1]. https://github.com/eclipse-openj9/openj9/blob/3d06b2f9c2c60e7655ec1beb6d6708fe5acf0642/runtime/compiler/z/codegen/S390PrivateLinkage.cpp#L104-L113
[2]. https://github.com/eclipse/omr/blob/cf8ddbd1adcd87e388cad6be1fd0e0c085285a29/compiler/z/codegen/OMRRegisterDependency.cpp#L539

@r30shah
Copy link
Contributor Author

r30shah commented Jun 15, 2022

@joransiu I have opened up OMR issue to work on migrating setting of those flags to OMR (#6572). Planning to work on that in coming weeks but as we are planning to start testing on VectorAPI, I do want to get this safeguard in to make sure we do not accidentally generate SIMD nodes on platform without vector facilities installed.

@r30shah r30shah mentioned this pull request Jun 15, 2022
Closed
@r30shah
Copy link
Contributor Author

r30shah commented Jun 15, 2022

@0xdaryl Can I request you to launch tests on this PR and if you are OK with it, merge as well?

@r30shah
Add safeguard check for SIMD ops support
3e03eaa 
This commit adds a check to getSupportsOpCodeForAutoSIMD to make sure
that SIMD opcode is supported only when we have support for Vector
instructions.

Signed-off-by: Rahil Shah <rahil@ca.ibm.com>
@0xdaryl
Copy link
Contributor

0xdaryl commented Jun 17, 2022

Jenkins build zlinux,zos

@0xdaryl 0xdaryl self-assigned this Jun 17, 2022
@0xdaryl 0xdaryl merged commit 9b49cb6 into eclipse-omr:master Jun 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@0xdaryl 0xdaryl 0xdaryl approved these changes

@fjeremic fjeremic Awaiting requested review from fjeremic

+1 more reviewer

@joransiu joransiu joransiu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@0xdaryl 0xdaryl

Labels

arch:z comp:compiler

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@r30shah @0xdaryl @joransiu