Add Fences after Volatile Field Stores #11262

r30shah · 2020-11-24T17:28:41Z

This commits ports changes to add Fences after volatile field stores which was committed to snapshot repository with additional changes to fix the compilation failures reported by assembler on z/OS.

Signed-off-by: Rahil Shah [email protected]

r30shah · 2020-11-24T17:33:09Z

FYI @fjeremic ,
As this change was originally committed by Filip and was merged to Snapshot repo after reviewing, I would like to request @joransiu to review and merge this change to master repo.
Changes in this PR depends on eclipse-omr/omr#5689

r30shah · 2020-11-24T17:39:00Z

Marking it as WIP as eclipse-omr/omr#5689 needs to be merged before this.

runtime/compiler/z/codegen/J9UnresolvedDataSnippet.hpp

r30shah · 2020-11-26T13:54:05Z

Removing WIP, OMR changes has been propagated. We can launch test on the changes.

r30shah · 2020-12-11T16:03:57Z

@fjeremic Can we launch tests on this change ?

fjeremic · 2020-12-11T17:27:05Z

Jenkins test sanity.functional,extended.functional,sanity.system,extended.system,special.system,sanity.openjdk zlinux,zlinuxxl jdk8

fjeremic · 2020-12-11T23:29:18Z

Testing looks good. I should remit myself as the committer for this one as I had a part in coding this change. @joransiu off to you!

genie-openj9 · 2021-05-17T14:17:17Z

Can one of the admins verify this patch?

r30shah · 2022-02-07T18:46:21Z

This change is still relevant. I will clean up and resolve merge conflicts to get this merged.

r30shah · 2023-06-22T14:07:28Z

@joransiu I have fixed merge conflicts for this test and also ran the sanity tests on it. Can you review and merge this changes?

r30shah · 2024-06-24T19:53:57Z

jenkins test sanity zlinux jdk17,jdk8

r30shah · 2024-06-25T14:22:16Z

@joransiu While talking to Spencer about unsafe get and set volatile methods, I remember this pending PR opened up some time ago, Can I request you to review this change? I have done sanity tests on this PR they are fine.

joransiu · 2024-06-28T18:44:52Z

Thanks @r30shah. Changes LGTM. I'm going to grab a PR test driver, and do some explicit rtResolve tests to manually double check the patching logic before merging.

joransiu · 2024-07-05T01:17:47Z

@r30shah : While reviewing some of the tree evaluations, I noticed that in the event of an awrtbari, the fence NOP is placed after the writebar code. The wrtbar code may actually call jitWriteBarrierStoreGenerational and incur significant pathlength before the fence is issued. Is that what we want?

; Live regs: GPR=2 FPR=0 VRF=0 {&GPR_0089, &GPR_0032}
------------------------------
 n74n     (  0)  ResolveCHK [#328]                                                                    [     0x3ff670806e0] bci=[-1,97,1268] rc=0 vc=193 vn=- li=3 udi=- nc=1
 n73n     (  2)    awrtbari  sun/nio/cs/StandardCharsets.cache Ljava/util/Map;[#400  unresolved notAccessed volatile Shadow] [flags 0x2607 0x0 ] (X!=0 )  [     0x3ff67080690] bci=[-1,97,1268] rc=2 vc=193 vn=- li=3 udi=- nc=3 flg=0x24
 n121n    (  2)      ==>aRegLoad (in &GPR_0032) (X!=0 X>=0 SeenRealReference )
 n141n    (  1)      l2i (X!=0 )                                                                      [     0x3ff67081bd0] bci=[-1,9,1261] rc=1 vc=193 vn=- li=- udi=- nc=1 flg=0x4
 n140n    (  1)        lushr (highWordZero X!=0 compressionSequence )                                 [     0x3ff67081b80] bci=[-1,9,1261] rc=1 vc=193 vn=- li=- udi=- nc=2 flg=0x4804
 n139n    (  1)          a2l (X!=0 unneededConv )                                                     [     0x3ff67081b30] bci=[-1,9,1261] rc=1 vc=193 vn=- li=- udi=- nc=1 flg=0x404
 n18n     (  2)            ==>new (in &GPR_0089) (X!=0 X>=0 )
 n138n    (  1)          iconst 2 (X!=0 X>=0 )                                                        [     0x3ff67081ae0] bci=[-1,97,1268] rc=1 vc=193 vn=- li=- udi=- nc=0 flg=0x104
 n121n    (  2)      ==>aRegLoad (in &GPR_0032) (X!=0 X>=0 SeenRealReference )
------------------------------
------------------------------
 n74n     (  0)  ResolveCHK [#328]                                                                    [     0x3ff670806e0] bci=[-1,97,1268] rc=0 vc=193 vn=- li=3 udi=- nc=1
 n73n     (  1)    awrtbari  sun/nio/cs/StandardCharsets.cache Ljava/util/Map;[#400  unresolved notAccessed volatile Shadow] [flags 0x2607 0x0 ] (X!=0 cannotOverflow storeAlreadyEvaluated )  [     0x3ff67080690] bci=[-1,97,1268] rc=1 vc=193 vn=- li=3 udi=- nc=3 flg=0x1024
 n121n    (  0)      ==>aRegLoad (in &GPR_0032) (X!=0 X>=0 SeenRealReference )
 n141n    (  0)      l2i (in GPR_0418) (X!=0 )                                                        [     0x3ff67081bd0] bci=[-1,9,1261] rc=0 vc=193 vn=- li=- udi=- nc=1 flg=0x4
 n140n    (  0)        lushr (in GPR_0418) (highWordZero X!=0 compressionSequence )                   [     0x3ff67081b80] bci=[-1,9,1261] rc=0 vc=193 vn=- li=- udi=- nc=2 flg=0x4804
 n139n    (  0)          a2l (in &GPR_0089) (X!=0 unneededConv )                                      [     0x3ff67081b30] bci=[-1,9,1261] rc=0 vc=193 vn=- li=- udi=- nc=1 flg=0x404
 n18n     (  1)            ==>new (in &GPR_0089) (X!=0 X>=0 )
 n138n    (  0)          iconst 2 (X!=0 X>=0 )                                                        [     0x3ff67081ae0] bci=[-1,97,1268] rc=0 vc=193 vn=- li=- udi=- nc=0 flg=0x104
 n121n    (  0)      ==>aRegLoad (in &GPR_0032) (X!=0 X>=0 SeenRealReference )
------------------------------

 [     0x3ff6720e550]                          LGR     &GPR_0417,&GPR_0032       ; LR=Clobber_eval
 [     0x3ff6720e6c0]                          SRLG    GPR_0418,&GPR_0089,2
 [     0x3ff6720f0f0]                          assocreg
 [     0x3ff6720eb00]                          BRCL    0xf, 0x000003FF6720E890
 POST:
 {GPR14:GPR_0420:R}
 [     0x3ff6720f860]                          assocreg
 [     0x3ff6720f1c0]                          ST      GPR_0418, Shadow[sun/nio/cs/StandardCharsets.cache Ljava/util/Map;] ?+350(&GPR_0032)
 POST:
 {AssignAny:&GPR_0032:R} {AssignAny:GPR_0418:R} {GPR14:GPR_0420:R}
 [     0x3ff6720fda0]                          LGR     GPR_0422,&GPR_0417
 [     0x3ff6720fe80]                          SG      GPR_0422,#466 216(GPR13)
 [     0x3ff67210000]                          CLG     GPR_0422,#467 224(GPR13)
 [     0x3ff672100d0]                          BRC     BL(0x2), Label L0102
 [     0x3ff67210330]                          TM      #468 421(GPR13), 0x10
 [     0x3ff67210410]                          BRC     BH(0x8), Label L0105
 [     0x3ff672104f0]                          SRLG    GPR_0422,GPR_0422,9
 [     0x3ff67210690]                          AG      GPR_0422,#469 200(GPR13)
 [     0x3ff67210820]                          MVI     #470 0(GPR_0422), 0x01
 [     0x3ff67210ec0]                          assocreg
 [     0x3ff67210900]                          Label L0105:
 POST:
 {GPR14:GPR_0423:R} {GPR1:&GPR_0417:R} {GPR2:&GPR_0089:R} {GPR4:GPR_0422:R}
 [     0x3ff67211550]                          assocreg
 [     0x3ff67210f90]                          Label L0104:
 POST:
 {GPR14:GPR_0423:R} {GPR1:&GPR_0417:R} {GPR2:&GPR_0089:R} {GPR4:GPR_0422:R}
 [     0x3ff672116e0]                          TM      #471 3(&GPR_0417), 0xf0
 [     0x3ff672117c0]                          BRC     MASK9(0x8), Snippet Label L0103
 [     0x3ff67211ed0]                          assocreg
 [     0x3ff67211910]                          Label L0102:
 POST:
 {GPR14:GPR_0423:R} {GPR1:&GPR_0417:R} {GPR2:&GPR_0089:R} {GPR4:GPR_0422:R}
 [     0x3ff67211fa0]                          NOP

r30shah · 2024-07-05T02:55:49Z

@joransiu Looking at the code, You are right, fence / serialization should happen after we store the object followed by the code for write barrier. I have made changes in https://github.com/eclipse-openj9/openj9/compare/2418be7df0e0613a0011339c5f59d22ca9ce069d..2a40088f00753716ebb5012b5b1c1d7f67cd4f75

I have converted this to WIP - Tomorrow will run tests and also try unit test to verify functionality with awrtbari.

We override the various store evaluators in OpenJ9 and emit a fence following volatile stores to adhere to the Java Memory Model. This is necessary for functional correctness. There are two cases to handle: 1. Field is resolved 2. Field is unresolved For 1. the implementation is trivial in that we simply emit the fence after the store evaluator has emitted the necessary instructions to evaluate the store node. For 2. the implementation is more complicated. We do not know at compile time whether the field in question is volatile or not. To combat this we emit a NOP placeholder and in the runtime resolution logic we call a VM helper to determine whether the resolved field is volatile or not. If we determine the field is volatile we overwrite the NOP in the mainline instruction stream with a fence. Co-authored-by: Filip Jeremic <[email protected]> Co-authored-by: Rahil Shah <[email protected]>

r30shah · 2024-07-05T18:30:27Z

@joransiu Thanks for catching the scenario under awrtbari. I have verified under unit test change in https://github.com/eclipse-openj9/openj9/compare/2a40088f00753716ebb5012b5b1c1d7f67cd4f75..56714e2310720269cac599b5426a9c54153dd6b0 and also trying to get internal builds (Some infrastructure issue is prohibiting me in getting the build to go on).

If you are OK with the change, I would go ahead and try to get testing on OpenJ9 jenkins.

joransiu

LGTM

r30shah · 2024-07-05T18:48:38Z

jenkins test sanity zlinux jdk17,jdk8

r30shah · 2024-07-10T12:28:21Z

@joransiu all the test has passed with this PR. This one is good to merge.

r30shah mentioned this pull request Nov 24, 2020

Supporting changes to add fences after volatile field stores eclipse-omr/omr#5689

Merged

r30shah force-pushed the fencesPR branch from ad9e0f1 to c97d10c Compare November 24, 2020 17:37

r30shah changed the title ~~Add Fences after Volatile Field Stores~~ WIP: Add Fences after Volatile Field Stores Nov 24, 2020

This was referenced Nov 24, 2020

Fences for Volatile Field Stores #11265

Closed

Fences for Volatile Field Stores #11266

Open

fjeremic requested a review from joransiu November 25, 2020 17:02

fjeremic added arch:z comp:jit labels Nov 25, 2020

fjeremic reviewed Nov 25, 2020

View reviewed changes

runtime/compiler/z/codegen/J9UnresolvedDataSnippet.hpp Outdated Show resolved Hide resolved

fjeremic approved these changes Nov 25, 2020

View reviewed changes

r30shah changed the title ~~WIP: Add Fences after Volatile Field Stores~~ Add Fences after Volatile Field Stores Nov 26, 2020

fjeremic self-assigned this Dec 11, 2020

fjeremic assigned joransiu and unassigned fjeremic Dec 11, 2020

r30shah force-pushed the fencesPR branch from 38de90f to 8d042e9 Compare September 1, 2022 16:00

r30shah force-pushed the fencesPR branch from 8d042e9 to 7b6fd19 Compare May 31, 2023 19:11

r30shah mentioned this pull request May 31, 2023

OpenJDK java/lang/Thread/virtual/stress/PingPong timeout #17255

Closed

r30shah force-pushed the fencesPR branch 4 times, most recently from e24235f to d09fb88 Compare June 21, 2023 20:40

r30shah force-pushed the fencesPR branch from d09fb88 to 2418be7 Compare June 24, 2024 19:52

r30shah requested a review from dsouzai as a code owner June 24, 2024 19:52

joransiu approved these changes Jun 28, 2024

View reviewed changes

r30shah force-pushed the fencesPR branch from 2418be7 to 2a40088 Compare July 5, 2024 02:54

r30shah changed the title ~~Add Fences after Volatile Field Stores~~ WIP: Add Fences after Volatile Field Stores Jul 5, 2024

r30shah force-pushed the fencesPR branch from 2a40088 to 56714e2 Compare July 5, 2024 13:00

r30shah changed the title ~~WIP: Add Fences after Volatile Field Stores~~ Add Fences after Volatile Field Stores Jul 5, 2024

joransiu approved these changes Jul 5, 2024

View reviewed changes

joransiu merged commit 92b291d into eclipse-openj9:master Jul 10, 2024
7 checks passed

Add Fences after Volatile Field Stores #11262

Add Fences after Volatile Field Stores #11262

Uh oh!

Conversation

r30shah commented Nov 24, 2020

Uh oh!

r30shah commented Nov 24, 2020

Uh oh!

r30shah commented Nov 24, 2020

Uh oh!

Uh oh!

r30shah commented Nov 26, 2020

Uh oh!

r30shah commented Dec 11, 2020

Uh oh!

fjeremic commented Dec 11, 2020

Uh oh!

fjeremic commented Dec 11, 2020

Uh oh!

genie-openj9 commented May 17, 2021

Uh oh!

r30shah commented Feb 7, 2022

Uh oh!

r30shah commented Jun 22, 2023

Uh oh!

r30shah commented Jun 24, 2024

Uh oh!

r30shah commented Jun 25, 2024

Uh oh!

joransiu commented Jun 28, 2024

Uh oh!

joransiu commented Jul 5, 2024

Uh oh!

r30shah commented Jul 5, 2024

Uh oh!

r30shah commented Jul 5, 2024

Uh oh!

joransiu left a comment

Choose a reason for hiding this comment

Uh oh!

r30shah commented Jul 5, 2024

Uh oh!

r30shah commented Jul 10, 2024

Uh oh!

Uh oh!

Uh oh!