RFR: 8351568: Improve source code documentation for PhaseCFG::insert_anti_dependences

[Galder Zamarreño]

On Mon, 28 Apr 2025 15:28:52 GMT, Daniel Lundén <dlunden at openjdk.org> wrote:

> The current documentation for `PhaseCFG::insert_anti_dependences` is difficult to follow and sometimes even misleading. We should ensure the method is appropriately documented.
> 
> ### Changeset
> 
> - Rename `PhaseCFG::insert_anti_dependences` to `PhaseCFG::raise_above_anti_dependences`. The purpose of `PhaseCFG::raise_above_anti_dependences` is twofold: raise the load's LCA so that the load is scheduled before anti-dependent stores, and if necessary add anti-dependence edges between the load and certain anti-dependent stores (to ensure we later "raise" the load before anti-dependent stores in LCM). The name `PhaseCFG::insert_anti_dependences` suggests that we only add anti-dependence edges. The name `PhaseCFG::raise_above_anti_dependences`, therefore, seems more appropriate.
> - Significantly add to and revise the source code documentation of `PhaseCFG::raise_above_anti_dependences`.
> - Add, move, and revise `assert`s in `PhaseCFG::raise_above_anti_dependences`, including improved `assert` messages in a few places.
> - In the main worklist loop of `PhaseCFG::raise_above_anti_dependences`:
>   - Clean up how we identify the search root (avoid mutation).
>   - Add a missing early exit for `Phi` nodes when `LCA == early`.
> 
> ### Testing
> 
> - [GitHub Actions](https://github.com/dlunde/jdk/actions/runs/14706896111)
> - `tier1` to `tier4` (and additional Oracle-internal testing) on Windows x64, Linux x64, Linux aarch64, macOS x64, and macOS aarch64.

Thanks @dlunde for the PR. Some small comments.

src/hotspot/share/opto/gcm.cpp line 889:

> 887:             // since the load will be forced into a block preceding the Phi.
> 888:             pred_block->set_raise_LCA_mark(load_index);
> 889:             assert(!LCA_orig->dominates(pred_block) ||

Has this assert moved elsewhere? Or do we really want to remove it altogether?

src/hotspot/share/opto/gcm.cpp line 912:

> 910:         // they CAN write to Java memory.
> 911:         if (muse->ideal_Opcode() == Op_CallStaticJava) {
> 912:           assert(muse->is_MachSafePoint(), "");

I know there was not assert message before, but can we use the opportunity to add a meaningful message for this assert? There's another empty message assert a few lines before.

test/hotspot/jtreg/compiler/loopopts/TestSplitIfPinnedLoadInStripMinedLoop.java line 141:

> 139: 
> 140:     // Same as test2 but with reference to inner loop induction variable 'j' and different order of instructions.
> 141:     // Triggers an assert in PhaseCFG::raise_above_anti_dependences if loop strip mining verification is disabled:

Is this test still valid? According to the comment it should trigger an assert but this assert appears to be removed? Is the test correct if the test is passing even though the assert has been removed? See my above comment on the removal of this assertion.

-------------

Changes requested by galder (Author).

PR Review: https://git.openjdk.org/jdk/pull/24926#pullrequestreview-2801983806
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2065453924
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2065450925
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2065456016