search

eclipse / omr

Eclipse OMR™ Cross platform components for building reliable, high performance language runtimes
http://www.eclipse.org/omr
Other
934 stars 392 forks source link

Kill prior store nodes in processing volatile store in local transparency #7341

Closed a7ehuo closed 1 month ago

a7ehuo commented 1 month ago

When processing a volatile store in TR_LocalTransparency::updateUsesAndDefs, it should kill any store node that has appeared earlier in the block. Otherwise, the store node could survive the block in local transparency but will be killed in _downwardExposedAnalysisInfo when the volatile store is processed in local anticipatability, which causes inconsistency between local transparency and local anticipatability. This matches how a call node is processed in TR_LocalTransparency::updateUsesAndDefs to set up symRefsDefinedAfterStored.

Fixes: eclipse-openj9/openj9#19218

a7ehuo commented 1 month ago

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

@hzongaro fyi

vijaysun-omr commented 1 month ago

Jenkins build all

vijaysun-omr commented 1 month ago

Amazing job narrowing down this longstanding PRE issue @a7ehuo Hugely impressed by your persistence and attention to detail in understanding the issue and coming up with a fix.

a7ehuo commented 1 month ago

The x86-64 macOS failure is a known issue https://github.com/eclipse/omr/issues/7181

...
2: ----------------------------------------------------------------------
2: Traceback (most recent call last):
2:   File "/Users/runner/work/1/s/jitbuilder/apigen/test/cppgentests.py", line 128, in test_to_opaque_cast_2
2:     self.assertRegexpMatches(self.generator.to_opaque_cast("bar", class_desc),
2:     ^^^^^^^^^^^^^^^^^^^^^^^^
2: AttributeError: 'CppGeneratorTest' object has no attribute 'assertRegexpMatches'
2: 
2: ----------------------------------------------------------------------
2: Ran 146 tests in 0.778s
2: 
2: FAILED (errors=25, skipped=3)
2: warning: The package 'jsonschema' is not installed so certain tests will be skipped
32/49 Test  #2: TestJitBuilderAPIGenerator ..........***Failed    1.00 sec
...
...
The following tests FAILED:
      2 - TestJitBuilderAPIGenerator (Failed)
Errors while running CTest
Output from these tests are in: /Users/runner/work/1/s/build/Testing/Temporary/LastTest.log
Use "--rerun-failed --output-on-failure" to re-run the failed cases verbosely.

##[error]Bash exited with code '8'.
Finishing: Test
vijaysun-omr commented 1 month ago

Okay, all other checks have passed. So I am merging.