Add Fences after Volatile Field Stores

[r30shah]

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]

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 https://github.com/eclipse/omr/pull/5689

[r30shah]

Marking it as WIP as https://github.com/eclipse/omr/pull/5689 needs to be merged before this.

[r30shah]

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

[r30shah]

@fjeremic Can we launch tests on this change ?

[fjeremic]

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

[fjeremic]

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]

Can one of the admins verify this patch?

[r30shah]

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

[r30shah]

@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]

jenkins test sanity zlinux jdk17,jdk8

[r30shah]

@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]

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]

@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]

@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.

[r30shah]

@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.

[r30shah]

jenkins test sanity zlinux jdk17,jdk8

[r30shah]

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