Compare nodes in addition to symRef in SequentialStoreSimplification

[rmnattas]

SequentialStoreSimplification compares baseVar (first child to aladd) symRef to confirm stores being to the same array for simplification. For object fields the symRef matches even if arrays are different.

To fix the issue a node check is added to make sure that its storing to the same array even if symRefs match.

n64n      bstorei  <array-shadow>[#222  Shadow] [flags 0x80000601 0x0 ]                       [0x7284a30603c0] bci=[-1,46,2542] rc=0 vc=730 vn=- li=- udi=- nc=2
n63n        aladd (X>=0 internalPtr )                                                         [0x7284a3060370] bci=[-1,46,2542] rc=1 vc=730 vn=- li=- udi=- nc=2 flg=0x8100
n8n           aloadi  TestSeqStore.a [B[#419  Shadow +8] [flags 0x607 0x0 ]                     [0x736004404ac0] bci=[-1,1,5] rc=6 vc=2185 vn=- li=- udi=- nc=1
n7n             aload  <parm 0 LTestSeqStore;>[#417  Parm] [flags 0x40000107 0x0 ] (X>=0 )      [0x736004404a70] bci=[-1,0,5] rc=2 vc=2185 vn=- li=- udi=13 nc=0 flg=0x100
n62n          lconst 9 (highWordZero X!=0 X>=0 )                                              [0x7284a3060320] bci=[-1,46,2542] rc=1 vc=730 vn=- li=- udi=- nc=0 flg=0x4104
n56n        bconst  93 (X!=0 X>=0 )                                                           [0x7284a3060140] bci=[-1,46,2542] rc=1 vc=730 vn=- li=- udi=- nc=0 flg=0x104
n76n      bstorei  <array-shadow>[#222  Shadow] [flags 0x80000601 0x0 ]                       [0x7284a3060780] bci=[-1,51,2543] rc=0 vc=730 vn=- li=- udi=- nc=2
n75n        aladd (X>=0 internalPtr )                                                         [0x7284a3060730] bci=[-1,51,2543] rc=1 vc=730 vn=- li=- udi=- nc=2 flg=0x8100
n19n          aloadi  TestSeqStore.a [B[#419  Shadow +8] [flags 0x607 0x0 ]                     [0x736004404e30] bci=[-1,10,5] rc=4 vc=2185 vn=- li=- udi=- nc=1
n18n            aload  <parm 1 LTestSeqStore;>[#418  Parm] [flags 0x40000107 0x0 ] (X>=0 )      [0x736004404de0] bci=[-1,9,5] rc=1 vc=2185 vn=- li=- udi=14 nc=0 flg=0x100
n74n          lconst 10 (highWordZero X!=0 X>=0 )                                             [0x7284a30606e0] bci=[-1,51,2543] rc=2 vc=730 vn=- li=- udi=- nc=0 flg=0x4104
n68n        bconst  13 (X!=0 X>=0 )                                                           [0x7284a3060500] bci=[-1,51,2543] rc=1 vc=730 vn=- li=- udi=- nc=0 flg=0x104

[rmnattas]

fyi @hzongaro
Testing PR

[vijaysun-omr]

I will wait for @hzongaro review and the WIP to be taken off before I consider merging.

[rmnattas]

Sequential Load Simplification is a part of SequentialStoreSimplification that @IBMJimmyk worked on.
He mentioned that it already does node comparison to verify base variable but for OffHeap it wouldn't allow tree shapes accessing first element given that it doesn't have an aladd.

I added another commit to this PR that solves that.

[vijaysun-omr]

May I request some printfs in the code to tell you how many times the sequential store elimination did transformations in a couple of the benchmarks that would run the opt with and without your changes ? If the opt only runs at hot and above, then you probably won't be able to run Liberty scenarios to gauge the effectiveness, and would need some other apps like SPECjbb2015 or ILOG or Spark.

I understand the optimization was doing a potentially incorrect transformation, but there may have been a case where it did not have commoned nodes and was still transforming correctly, but now no longer does so. I don't want to delay your changes too much, so you can fix the functional issue first, and then follow up with performance evaluation in the near term and that would be okay with me.

[rmnattas]

I'll lookup on the load simplification changes necessary and may decide to put do it separately after instead to get this fix going. Load simplification will be functionally correct but not being able to transform the specific case of first element accesses.

I'll add the printfs and measure it after completing the fix.

[rmnattas]

PR ready for merge. Testing performance I don't see difference in running benchmarks.

[vijaysun-omr]

Jenkins test sanity.functional,sanity.openjdk jdk21

[hzongaro]

Jenkins test sanity.functional,sanity.openjdk xlinux jdk21

[vijaysun-omr]

Testing has passed, @hzongaro are you thinking of approving this ? I ask, to check before I consider a merge

[hzongaro]

I think this looks good. Thanks!

[hzongaro]

are you thinking of approving this?

Sorry for the delay - I had just finished reviewing.

[hzongaro]

Jenkins test sanity.functional,sanity.openjdk xlinux jdk21