RFR: 8351568: Improve source code documentation for PhaseCFG::insert_anti_dependences [v2]

Wed May 7 14:55:29 UTC 2025

On Wed, 30 Apr 2025 10:17:34 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.
>
> Daniel Lundén has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Updates after reviews

Thanks for doing this, Daniel! `insert_anti_dependences` is indeed easier to understand after your proposed cleanups and additional comments. I have a few questions and suggestions.

Please update also the reference to `insert_anti_dependencies` in `src/hotspot/share/adlc/output_h.cpp`.

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

> 665: 
> 666: //------------------------raise_above_anti_dependences---------------------------
> 667: // The argument load has a current scheduling range in the dominator tree that

Could you start this long comment with a one-sentence summary of what the function does? I.e. something like "Enforce a scheduling of the given load where its input memory state is not overwritten by an anti-dependent store".

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

> 708: // B4, which means that the updated LCA is B2. Now, consider the store in B2.
> 709: // Raising the LCA above B2 has no effect, because B2 is on the dominator tree
> 710: // branch between early and the current LCA (in fact, B2 is the current LCA).

I found this sentence a bit unclear, could you clarify what you mean by "has no effect"?

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

> 721: // edges back to the load. The caller is expected to eventually schedule the
> 722: // load in the LCA, but may also hoist the load above the LCA, if it is not the
> 723: // early block.

What code expects the caller to schedule the load in the LCA? Maybe rephrase into something more relaxed like e.g. "The caller may schedule the load in the LCA, or it may hoist the load above the LCA, if it is not the early block.".

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

> 756: 
> 757:   // Note the earliest legal placement of 'load', as determined by
> 758:   // by the unique point in the dominator tree where all memory effects

Suggestion:

  // the unique point in the dominator tree where all memory effects

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

> 777:   ResourceArea* area = Thread::current()->resource_area();
> 778: 
> 779:   // Bookkeeping of possibly anti-dependent stores that we find outside of the

Suggestion:

  // Bookkeeping of possibly anti-dependent stores that we find below the

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

> 783:   Node_List non_early_stores(area);
> 784: 
> 785:   // Flag that indicates if we must attempt to raise the LCA after the main

Suggestion:

  // Whether we must attempt to raise the LCA after the main

Also, could you clarify what you mean by "attempt"? Could LCA raising fail somehow?

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

> 801:   // MergeMems do not modify the memory state. Anti-dependent stores or memory
> 802:   // Phis may, however, exist downstream of MergeMems. Therefore, we must
> 803:   // permit the search to continue through MergeMems. Memory-state-modifying

Now that you have already explained that "memory-state-modifying nodes" are also referred to as "stores", you could stick to using "stores" for brevity.

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

> 848:     // - just past a MergeMem with the edge (MergeMem, use_mem_state).
> 849:     // we have passed a MergeMem and are now at an edge
> 850:     // (MergeMem, use_mem_state).

Are these two lines intended to be here?

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

> 851:     assert(def_mem_state == nullptr || def_mem_state == initial_mem ||
> 852:                def_mem_state->is_MergeMem(),
> 853:            "invariant failed");

Suggestion:

           "unexpected memory state");

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

> 890: 
> 891:     // At this point, use_mem_state is either a store or a memory Phi.
> 892:     assert(!use_mem_state->is_MergeMem(), "invariant failed");

Suggestion:

    assert(!use_mem_state->is_MergeMem(),
           "use_mem_state should be either a store or a memory Phi");

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

> 949:     // which we must raise the LCA above (set_raise_LCA_mark), and keep
> 950:     // track of nodes that potentially need anti-dependence edges
> 951:     // (non_early_stores). The only exceptions to this is if we

Suggestion:

    // (non_early_stores). The only exceptions to this are if we

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

> 955:     //
> 956:     // After the worklist loop, we perform an efficient combined LCA-raising
> 957:     // operation over all marks and then only add anti-dependence edges where

Suggestion:

    // operation over all marks and only then add anti-dependence edges where

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

> 1012:             pred_block->set_raise_LCA_mark(load_index);
> 1013:             must_raise_LCA = true;
> 1014:           } else /* if (pred_block == early */ {

Suggestion:

          } else /* if (pred_block == early) */ {

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

> 1050:     }
> 1051:   }
> 1052:   // (Worklist is now empty; we have visited all possible anti-dependences.)

Suggestion:

  // Worklist is now empty; we have visited all possible anti-dependences.

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:     // Triggered an assert in PhaseCFG::raise_above_anti_dependences if loop strip mining verification was disabled:

If the proposed assertions are stronger than the one in mainline, there is no need to rewrite this sentence in past tense, in my opinion.

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

PR Review: https://git.openjdk.org/jdk/pull/24926#pullrequestreview-2821989713
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2077769631
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2077771523
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2077772673
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2077774045
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2077776098
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2077778886
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2077783735
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2077786973
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2077789623
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2077792516
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2077793880
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2077794806
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2077796052
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2077813208
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2077819729