Add the missing live locals info to the new blocks in tree lowering

[a7ehuo]

When the tree is transformed into new blocks in fastpathAcmpHelper or lowerArrayStoreCHK, the live locals info is not passed on from the previous block to the newly created blocks. The missing live locals causes missing GC map for the objects.

Many thanks to @liqunl for finding the cause of the missing GC map.

Depends on:

• WIP: Copy liveLocals to the new block in splitPostGRA eclipse-omr/omr#5639

Fixes #10869

[a7ehuo]

@liqunl @andrewcraik Ready for review. Thanks!

[Leonardo2718]

It's possible this PR also fixes #10365

[liqunl]

This will only fix gc map issue caused by ValueType lowering. Any opt that adds new blocks to the CFG after GRA (jprofiling is another example) will have the same problem. The reason is that GRA runs liveness analysis, and setLiveLocals on codegen to indicate there is liveness info on blocks, such that codegen can use it to compute gc map. As more and more complicated opts are added to post-GRA, this problem may become more relevant. And it's hard for a developer to tell the invisible dependency between GRA and gc map computation.

Maybe we should run liveness analysis in codegen to get a correct and most-recent liveness info before computing gc map. @andrewcraik What do you think?

[andrewcraik]

@liqunl I am not sure if liveness analysis will work once the optimizer is shutdown and the trees have started to be transformed ready for evaluation. You could experiment with running it since it seems reasonable, but I am not certain it will work and we may have to fix up the liveness. We need an issue to note all the places that need to be fixed.

[a7ehuo]

@liqunl @andrewcraik For my learning development, I'm curious of how the current liveness analysis is done. Could you point me to where the code (which file or function) is? If liveness analysis is moved to codegen, would this PR patching up fastpathAcmpHelper and lowerArrayStoreCHK still be required?

[liqunl]

GRA computes the liveness here https://github.com/eclipse/omr/blob/master/compiler/optimizer/GlobalRegisterAllocator.cpp#L398-L447
I tried liveness in codegen by copying code from GRA (liqunl@24ca976) , but got a failure in my pbuild. I think it might be due to block liveness info not cleaned up. Could you try cleaning up liveness info before running liveness analysis in codegen, see if that works?

[a7ehuo]

I tried setLiveLocals(NULL) for blocks and codegen along with the code liqunl/openj9@24ca976 that moves the liveness analysis to codegen. I ran into issues as below.

comp->getFlowGraph()->getStructure() could be NULL in codegen. It is required by the liveness analysis. Recomputing structure [1] in codegen doesn’t work. An assert in GC occurs [2]. If I skip the the liveness analysis when structure is NULL, there is another assert in GC [3], more or less the same assert.

[1]

   if (!comp->getFlowGraph()->getStructure())
      {
      comp->getFlowGraph()->setStructure(TR_RegionAnalysis::getRegions(comp));
      }
   // liveness analysis as GRA

[2]

18:10:32.046 0x19a00    j9mm.479    *   ** ASSERTION FAILED ** at /root/home/ahuo/src/openj9-openjdk-jdk11/openj9/runtime/gc_glue_java/ScavengerRootScanner.hpp:108: ((MM_StackSlotValidator(MM_StackSlotValidator::NOT_ON_HEAP, *slotPtr, stackLocation, walkState).validate(_env)))

[3]

15:57:24.890 0x19a00    j9mm.479    *   ** ASSERTION FAILED ** at /root/home/ahuo/src/openj9-openjdk-jdk11/openj9/runtime/gc_glue_java/ConcurrentMarkingDelegate.cpp:61: ((MM_StackSlotValidator(MM_StackSlotValidator::NOT_ON_HEAP, object, stackLocation, walkState).validate(data->env)))

[liqunl]

@a7ehuo Thanks for the experiments. It turned out there are more problems than we thought in doing liveness info in codegen, I feel like it should be doable because GlobalLiveVariablesForGC will lower trees before doing liveness analysis, so the tree shape should be similar to the one in codegen. But we can look at this approach later when manually fixing liveness becomes too tedious or impossible.

[a7ehuo]

Thanks for the example @liqunl. I added the following two changes during liveness analysis from LiveVariablesForGC.cpp in a7ehuo/openj9@0eab767 on top of liqunl/openj9@24ca976. I no longer see the above asserts. I'm doing more tests and see how it goes.

https://github.com/eclipse/openj9/blob/c18472815f59d96865211b54583153db9bceb351/runtime/compiler/optimizer/LiveVariablesForGC.cpp#L208-L215
https://github.com/eclipse/openj9/blob/c18472815f59d96865211b54583153db9bceb351/runtime/compiler/optimizer/LiveVariablesForGC.cpp#L269-L285

[a7ehuo]

Looks like adding the above two changes does not solve the issue. Ran sanity.functional test with a7ehuo/openj9@0eab767, thej9mm.479 asserts from GC still happens intermittently.

[a7ehuo]

@andrewcraik With the test/experiment that's done so far on moving the liveness analysis to codegen, there are more things to consider to make it work. Could you review this PR if we should take this manual fix in tree lowering for now? Thanks!

[andrewcraik]

I am not sure if running/rerunning that analysis is the right fix. The tactical fix seems more appropriate for the time being.

[a7ehuo]

Close this PR. The issue is addressed by #11214