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

Do not swing down volatile nodes #7281

Merged

Conversation

a7ehuo
Copy link
Contributor

@a7ehuo a7ehuo commented Mar 6, 2024

Volatile nodes should always be anchored under a treetop to avoid any interference with other optimization passes (e.g. PRE).

Fixes eclipse-openj9/openj9#18777

Volatile nodes should always be anchored under a treetop
to avoid any interference with other optimization passes
(e.g. PRE).

Fixes eclipse-openj9/openj9#18777

Signed-off-by: Annabelle Huo <Annabelle.Huo@ibm.com>
@a7ehuo
Copy link
Contributor Author

a7ehuo commented Mar 6, 2024

An example of with this change

Without this change (baseline), a load of volatile symbol n94n is eventually buried down under a store node n98n

n95n      treetop                                                                             [0x7fa5dd46fd70] bci=[-1,42,12] rc=0 vc=270 vn=- li=-1 udi=- nc=1
n92n        iloadi  Test.b I[#425  Shadow +8] [flags 0x603 0x0 ] (cannotOverflow )            [0x7fa5dd46fc80] bci=[-1,38,12] rc=2 vc=270 vn=- li=- udi=- nc=1 flg=0x1000
n91n          aload  <'this' parm LTest;>[#422  Parm] [flags 0x40000107 0x0 ] (X!=0 X>=0 )    [0x7fa5dd46fc30] bci=[-1,36,12] rc=2 vc=270 vn=- li=- udi=- nc=0 flg=0x104
n98n      istorei  Test.b I[#425  Shadow +8] [flags 0x603 0x0 ]                               [0x7fa5dd46fe60] bci=[-1,46,12] rc=0 vc=270 vn=- li=-1 udi=- nc=2
n91n        ==>aload
n97n        ishr                                                                              [0x7fa5dd46fe10] bci=[-1,45,12] rc=1 vc=270 vn=- li=- udi=- nc=2
n92n          ==>iloadi
n94n          iloadi  Test.c B[#446  volatile Shadow +12] [flags 0x2601 0x0 ] (cannotOverflow )  [0x7fa5dd46fd20] bci=[-1,42,12] rc=1 vc=270 vn=- li=- udi=- nc=1 flg=0x1000
n93n            aload  <'this' parm LTest;>[#422  Parm] [flags 0x40000107 0x0 ] (X!=0 X>=0 )  [0x7fa5dd46fcd0] bci=[-1,41,12] rc=1 vc=270 vn=- li=- udi=- nc=0 flg=0x104
n101n    

With this change, a load of volatile symbol n94n remains under the treetop node.

n95n      treetop                                                                             [0x7f26a0985d70] bci=[-1,42,12] rc=0 vc=270 vn=- li=-1 udi=- nc=1
n92n        iloadi  Test.b I[#425  Shadow +8] [flags 0x603 0x0 ] (cannotOverflow )            [0x7f26a0985c80] bci=[-1,38,12] rc=2 vc=270 vn=- li=- udi=- nc=1 flg=0x1000
n91n          aload  <'this' parm LTest;>[#422  Parm] [flags 0x40000107 0x0 ] (X!=0 X>=0 )    [0x7f26a0985c30] bci=[-1,36,12] rc=2 vc=270 vn=- li=- udi=- nc=0 flg=0x104
n96n      treetop                                                                             [0x7f26a0985dc0] bci=[-1,42,12] rc=0 vc=270 vn=- li=-1 udi=- nc=1
n94n        iloadi  Test.c B[#446  volatile Shadow +12] [flags 0x2601 0x0 ] (cannotOverflow )  [0x7f26a0985d20] bci=[-1,42,12] rc=2 vc=270 vn=- li=- udi=- nc=1 flg=0x1000
n93n          aload  <'this' parm LTest;>[#422  Parm] [flags 0x40000107 0x0 ] (X!=0 X>=0 )    [0x7f26a0985cd0] bci=[-1,41,12] rc=1 vc=270 vn=- li=- udi=- nc=0 flg=0x104
n98n      istorei  Test.b I[#425  Shadow +8] [flags 0x603 0x0 ]                               [0x7f26a0985e60] bci=[-1,46,12] rc=0 vc=270 vn=- li=-1 udi=- nc=2
n91n        ==>aload
n97n        ishr                                                                              [0x7f26a0985e10] bci=[-1,45,12] rc=1 vc=270 vn=- li=- udi=- nc=2
n92n          ==>iloadi
n94n          ==>iloadi

@a7ehuo
Copy link
Contributor Author

a7ehuo commented Mar 6, 2024

@vijaysun-omr May I ask you to review this change? Thank you!

@hzongaro fyi

@vijaysun-omr
Copy link
Contributor

jenkins build all

@vijaysun-omr vijaysun-omr self-assigned this Mar 7, 2024
@vijaysun-omr
Copy link
Contributor

jenkins build win

@vijaysun-omr
Copy link
Contributor

Test have passed apart from known issues. The change is a conservative one that is cross platform. Merging.

@vijaysun-omr vijaysun-omr merged commit 6625248 into eclipse:master Mar 7, 2024
16 of 18 checks passed
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.

OpenJ9 JDK8 Produces Different Results from Oracle JDK
2 participants