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.

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

Implement initial Vector API expansion #13213

Merged
merged 1 commit into from
Aug 30, 2021
Merged

Implement initial Vector API expansion #13213

merged 1 commit into from
Aug 30, 2021

Conversation

gita-omr
Copy link
Contributor

@gita-omr gita-omr commented Jul 19, 2021 •
edited
Loading

  • create new optimization pass(VectorAPIExpansion)
  • add VectorAPIExpansion to the cheap warm optimization strategy
  • recognize and scalarize load(), store() and binaryOp() intrinsics
  • add -Xjit command line option disableVectorAPIExpansion
  • add -Xjit command line option traceVectorAPIExpansion

@gita-omr gita-omr changed the title Implement initial Vector API expansion WIP:Implement initial Vector API expansion Jul 19, 2021
@gita-omr
Copy link
Contributor Author

Made WIP due to 2 sanity failures which are most likely not related. Investigating.

@gita-omr gita-omr added this to the Release 0.28 (Java 17) milestone Jul 19, 2021
@gita-omr gita-omr changed the title WIP:Implement initial Vector API expansion Implement initial Vector API expansion Jul 20, 2021
@gita-omr
Copy link
Contributor Author

Removed WIP since similar failures happened without this PR. Hoping somebody can take a look soon since there is a lot of code and we would like to get this into 0.28.

@gita-omr
Copy link
Contributor Author

@vijaysun-omr @0xdaryl @jdmpapin @r30shah @BradleyWood could you please review as much as you can?

@gita-omr gita-omr changed the title Implement initial Vector API expansion WIP: Implement initial Vector API expansion Jul 26, 2021
@gita-omr
Copy link
Contributor Author

Noticed some GRA related issues. Fixing.

@gita-omr gita-omr changed the title WIP: Implement initial Vector API expansion Implement initial Vector API expansion Jul 27, 2021
@gita-omr
Copy link
Contributor Author

Added commit with some fixes.

@gita-omr gita-omr requested review from 0xdaryl and fjeremic and removed request for vijaysun-omr July 27, 2021 13:32
@gita-omr gita-omr changed the title Implement initial Vector API expansion WIP: Implement initial Vector API expansion Jul 28, 2021
@gita-omr
Copy link
Contributor Author

Changed to WIP to make sure Byte and Short types work properly.

@gita-omr
Copy link
Contributor Author

Added a commit to handle Byte and Short types. As @jdmpapin pointed out there were some issues with those. I decided to use the type promotion approach (see commit message).

@gita-omr gita-omr changed the title WIP: Implement initial Vector API expansion Implement initial Vector API expansion Jul 29, 2021
@gita-omr gita-omr changed the title Implement initial Vector API expansion WIP:Implement initial Vector API expansion Aug 6, 2021
@gita-omr
Copy link
Contributor Author

gita-omr commented Aug 6, 2021

WIP: several fixes are coming and also would like to squash all the commits before merging.

@gita-omr
Copy link
Contributor Author

gita-omr commented Aug 8, 2021

Thanks a lot for all the findings and suggestions!
I think I addressed all the issues except for the Java version. I could not find any examples in the code where we check it. Usually, features are based on some build macro or command line option. In the case of Vector API acceleration, I think it should be enabled by default, at least starting with Java 16. I was planning to use ifVectorAPI flag[1] that would be set during Inlining. This flag will be applied to the optimization itself as well as to the optimizations that will need to be added. Currently, the overhead is a quick pass through the treetops looking for the Vector API methods.

Please let me know what you think. @r30shah @jdmpapin @BradleyWood @0xdaryl @vijaysun-omr

[1] eclipse/omr#6117

@vijaysun-omr
Copy link
Contributor

vijaysun-omr commented Aug 8, 2021 •
edited
Loading

Setting and checking IfVector sounds reasonable to me; a walk over the trees to collect this info may or may not even be needed, e.g. if the flag was set during IL gen itself (I believe this is how other flags are managed, e.g. IfNews) as the trees are being generated.

Regardless, a walk over IL trees (if you figure that is what we need in the end) is also not really significant overhead even at warm.

I assume there would be an option to disable all the vector handling at once.

@gita-omr
Copy link
Contributor Author

gita-omr commented Aug 9, 2021

Depends on eclipse/omr#6136

@gita-omr
Copy link
Contributor Author

eclipse/omr#6136 passed acceptance builds, so removing WIP.

@gita-omr gita-omr changed the title WIP:Implement initial Vector API expansion Implement initial Vector API expansion Aug 11, 2021
@fjeremic
Copy link
Contributor

@gita-omr could you take a look at the linter + line endings job failures? They look related to this PR. Once we have that fixed we can launch sanity tests + any Vector API testing we have available via PR triggers.

@gita-omr
Copy link
Contributor Author

I believe Linter should pass now since eclipse/omr#6136 got propagated.

@gita-omr
Copy link
Contributor Author

Removed training spaces in all files from this PR. Please let me know when I can squash all the commits.

@fjeremic
Copy link
Contributor 

12:34:15  + git diff --check origin/master HEAD
12:34:15  runtime/compiler/optimizer/VectorAPIExpansion.cpp:1381: new blank line at EOF.

Still one left. We should squash after that.

@gita-omr
Copy link
Contributor Author

runtime/compiler/optimizer/VectorAPIExpansion.cpp:1381: new blank line at EOF.

Wow, my sed script did not catch that :)

@fjeremic
Copy link
Contributor

Jenkins test sanity all jdk8,jdk17

@gita-omr
Copy link
Contributor Author

I am fixing AIX(XLC) build issue.

@gita-omr
Copy link
Contributor Author

Fixed xlC errors. Please let me know when to squash again.

@fjeremic
Copy link
Contributor

Jenkins test sanity all jdk8,jdk17

@gita-omr
Implement initial Vector API expansion
c29d6e6 
- create new optimization pass(VectorAPIExpansion)
- add VectorAPIExpansion to the cheap warm optimization strategy
- recognize and scalarize load(), store() and binaryOp() intrinsics
- add -Xjit command line option disableVectorAPIExpansion
- add -Xjit command line option traceVectorAPIExpansion
@gita-omr
Copy link
Contributor Author

Squashed and rebased.

@fjeremic
Copy link
Contributor

Jenkins test sanity all jdk8,jdk17

@fjeremic
Copy link
Contributor

Jenkins test sanity win jdk8

@fjeremic fjeremic self-assigned this Aug 30, 2021
@fjeremic
Copy link
Contributor

Windows 32-bit failure is fixed via #13406.

@fjeremic fjeremic merged commit 368d39c into eclipse-openj9:master Aug 30, 2021
@gita-omr gita-omr mentioned this pull request Aug 30, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BradleyWood BradleyWood BradleyWood left review comments

@r30shah r30shah r30shah left review comments

@jdmpapin jdmpapin jdmpapin left review comments

@fjeremic fjeremic fjeremic approved these changes

@0xdaryl 0xdaryl Awaiting requested review from 0xdaryl

Assignees

@fjeremic fjeremic

Labels
comp:jit
Projects
None yet
Milestone
Release 0.29 (Java 8, 11, 17) Oct ref...
Development

Successfully merging this pull request may close these issues.
7 participants
@gita-omr @vijaysun-omr @fjeremic @BradleyWood @r30shah @jdmpapin @0xdaryl