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

VectorAPI analyze guard for VectorAPI package #16354

Merged
merged 1 commit into from
Nov 29, 2022
Merged

VectorAPI analyze guard for VectorAPI package #16354

merged 1 commit into from
Nov 29, 2022

Conversation

r30shah
Copy link
Contributor

@r30shah r30shah commented Nov 22, 2022

If the Virtual guards is generated to guard a inlined method from Vector API package, set them to do not kill fear.

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

@r30shah r30shah marked this pull request as ready for review November 22, 2022 15:33
@r30shah r30shah changed the title WIP: VectorAPI analyze guard for VectorAPI package VectorAPI analyze guard for VectorAPI package Nov 24, 2022
@r30shah
Copy link
Contributor Author

r30shah commented Nov 24, 2022

@jdmpapin / @vijaysun-omr Can I please get review on this refinement changes over the previous one where merging was disable if there is any Vector API intrinsic call in method.

@gita-omr For now I have included two packages for the Vector JEP, do you know if there are any other Class we should include?

On IBM Z I tried out this change with vector jep benchmarks and I do see all the supported operations being vectorized.

jdmpapin
jdmpapin reviewed Nov 25, 2022
View reviewed changes
Copy link
Contributor

@jdmpapin jdmpapin left a comment

Choose a reason for hiding this comment

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

The overall strategy is good, as we discussed previously. I have one particularly important comment (about classNameLength()), and a number of requests for small improvements

runtime/compiler/optimizer/FearPointAnalysis.cpp Outdated Show resolved Hide resolved
runtime/compiler/optimizer/FearPointAnalysis.cpp Outdated Show resolved Hide resolved
runtime/compiler/optimizer/OSRGuardInsertion.cpp Outdated Show resolved Hide resolved
runtime/compiler/optimizer/HCRGuardAnalysis.cpp Outdated Show resolved Hide resolved
runtime/compiler/optimizer/FearPointAnalysis.cpp Outdated Show resolved Hide resolved
runtime/compiler/optimizer/FearPointAnalysis.cpp Outdated Show resolved Hide resolved
runtime/compiler/optimizer/FearPointAnalysis.cpp Outdated Show resolved Hide resolved
@gita-omr
Copy link
Contributor

@gita-omr For now I have included two packages for the Vector JEP, do you know if there are any other Class we should include?

No, I don't know of any other packages. Actually, even just "jdk/incubator/vector" might be enough but it's ok to keep both.

@r30shah r30shah force-pushed the OSRGuardVectorJEPAnalysis branch 2 times, most recently from bc2248f to b58ab5a Compare November 25, 2022 16:39
jdmpapin
jdmpapin reviewed Nov 25, 2022
View reviewed changes
Copy link
Contributor

@jdmpapin jdmpapin left a comment

Choose a reason for hiding this comment

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

Thanks for your updates - a few more comments

runtime/compiler/optimizer/FearPointAnalysis.cpp Outdated Show resolved Hide resolved
runtime/compiler/optimizer/FearPointAnalysis.cpp Outdated Show resolved Hide resolved
runtime/compiler/optimizer/FearPointAnalysis.cpp Outdated Show resolved Hide resolved
@r30shah r30shah force-pushed the OSRGuardVectorJEPAnalysis branch 2 times, most recently from fb04eef to d2a57e7 Compare November 25, 2022 18:06
@jdmpapin
Copy link
Contributor

LGTM. Please squash

@r30shah r30shah force-pushed the OSRGuardVectorJEPAnalysis branch from 9c03ca5 to 90c5be8 Compare November 25, 2022 21:43
@vijaysun-omr
Copy link
Contributor

Jenkins test sanity all jdk8,jdk11,jdk19

@r30shah r30shah force-pushed the OSRGuardVectorJEPAnalysis branch from 90c5be8 to 58b7270 Compare November 28, 2022 14:00
@vijaysun-omr
Copy link
Contributor

Jenkins test sanity all jdk11,jdk19

@r30shah r30shah changed the title VectorAPI analyze guard for VectorAPI package WIP: VectorAPI analyze guard for VectorAPI package Nov 28, 2022
@r30shah
Copy link
Contributor Author

r30shah commented Nov 28, 2022 •
edited
Loading

I am seeing assert being hit in the personal build. Checking out the path it takes. Marking this WIP till I verify
FYI @jdmpapin

Node 0x7f20c6b05960 [ificmpne]: HCR guard should generate fear and should not be considered to kill it.

09:53:41  Node context:
09:53:41  
09:53:41  ...
09:53:41  n2232n    ificmpne --> block_125 BBStart at n2228n (HCRGuard/NonoverriddenTest )              [0x7f20c6b05960] bci=[24,0,46] rc=0 vc=0 vn=- li=- udi=- nc=2 flg=0x1020
09:53:41  n2230n      iload  unknown static[#717  Static] [flags 0x10303 0x0 ]                          [0x7f20c6b058c0] bci=[20,129,1703] rc=1 vc=0 vn=- li=- udi=- nc=0
09:53:41  n2231n      iconst 0                                                                          [0x7f20c6b05910] bci=[20,129,1703] rc=1 vc=0 vn=- li=- udi=- nc=0

@jdmpapin
Copy link
Contributor

Summary of my discussion with @r30shah: The assertion I suggested was too aggressive. We remove all (removable) HCR guards before doing the fear analysis, so at the time of the fear analysis, all HCR guards that appear in the trees will remain, and it should be fine for them to kill fear.

Sorry @r30shah for leading you astray on that 😅

@r30shah r30shah force-pushed the OSRGuardVectorJEPAnalysis branch from 58b7270 to 1dd2e18 Compare November 28, 2022 15:54
@r30shah
Copy link
Contributor Author

r30shah commented Nov 28, 2022

I have removed the FATAL ASSERT in https://github.com/eclipse-openj9/openj9/compare/58b7270c739132febafa4222d5205e0e043d7b40..1dd2e18a414a823b658f8d53245785f3d0759444
before removing WIP will do another quick sanity build to make sure.

@r30shah r30shah closed this Nov 28, 2022
@r30shah r30shah reopened this Nov 28, 2022
@r30shah
VectorAPI analyze guard for VectorAPI package
50998f9 
If the Virtual guards is generated to guard a inlined method from Vector
API package, set them to do not kill fear.

Signed-off-by: Rahil Shah <rahil@ca.ibm.com>
@r30shah r30shah force-pushed the OSRGuardVectorJEPAnalysis branch from 1dd2e18 to 50998f9 Compare November 28, 2022 19:51
@jdmpapin
Copy link
Contributor

Jenkins test sanity all jdk8,jdk11,jdk19

@r30shah r30shah changed the title WIP: VectorAPI analyze guard for VectorAPI package VectorAPI analyze guard for VectorAPI package Nov 28, 2022
@r30shah
Copy link
Contributor Author

r30shah commented Nov 28, 2022 •
edited
Loading

removing WIP as sanity build : job/Test_openjdk17_j9_sanity.functional_x86-64_linux_Personal/524/ passes

@r30shah r30shah mentioned this pull request Nov 28, 2022
Merged
@vijaysun-omr
Copy link
Contributor

Checks have passed. Merging this now.

@vijaysun-omr vijaysun-omr merged commit 3f2deca into eclipse-openj9:master Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdmpapin jdmpapin jdmpapin approved these changes

@vijaysun-omr vijaysun-omr vijaysun-omr approved these changes

Assignees
No one assigned
Labels
comp:jit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@r30shah @gita-omr @jdmpapin @vijaysun-omr