RFR: 8351568: Improve source code documentation for PhaseCFG::insert_anti_dependences [v6]
Christian Hagedorn
chagedorn at openjdk.org
Mon May 12 13:11:06 UTC 2025
On Fri, 9 May 2025 18:10:09 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:
>
> Update src/hotspot/share/opto/gcm.cpp
>
> Co-authored-by: Roberto Castañeda Lozano <robcasloz at users.noreply.github.com>
Nice documentation! A lot of minor comments/suggestions while reading through the comments in the entire method.
src/hotspot/share/opto/gcm.cpp line 666:
> 664: };
> 665:
> 666: //------------------------raise_above_anti_dependences---------------------------
These legacy headers can be removed when we touch them.
Suggestion:
src/hotspot/share/opto/gcm.cpp line 667:
> 665:
> 666: //------------------------raise_above_anti_dependences---------------------------
> 667: // Enforce a scheduling of the argument load that ensures anti-dependent stores
I suggest to add `'` to make a mapping to the parameter:
Suggestion:
// Enforce a scheduling of the given 'load' that ensures anti-dependent stores
Same on L670 but somehow I cannot make a suggestion there. Maybe due to the existing PR comment?
src/hotspot/share/opto/gcm.cpp line 672:
> 670: // The argument load has a current scheduling range in the dominator tree that
> 671: // starts at the load's early block (computed in schedule_early) and ends at
> 672: // the argument LCA block. However, there may still exist anti-dependent stores
argument = load?
Suggestion:
// the load's LCA block. However, there may still exist anti-dependent stores
src/hotspot/share/opto/gcm.cpp line 673:
> 671: // starts at the load's early block (computed in schedule_early) and ends at
> 672: // the argument LCA block. However, there may still exist anti-dependent stores
> 673: // in between the early block and the LCA that overwrite memory that the load
Suggestion:
// between the early block and the LCA that overwrite memory that the load
src/hotspot/share/opto/gcm.cpp line 679:
> 677: // latest in the store's block, and
> 678: // 2. if the load may get scheduled in the store's block, additionally insert
> 679: // an anti-dependence edge from the load to the store to ensure LCM
Maybe you can also mention here that this is done by adding a precedence edge:
Suggestion:
// an anti-dependence edge (i.e. precedence edge) from the load to the store to ensure LCM
src/hotspot/share/opto/gcm.cpp line 685:
> 683: // path relative to the load if there are no paths from early to LCA that go
> 684: // through the store's block. Such stores are not anti-dependent, and there is
> 685: // no need to update the LCA nor to add anti-dependence edges.
Suggestion:
// no need to update the load's LCA nor to add anti-dependence edges.
src/hotspot/share/opto/gcm.cpp line 721:
> 719: //
> 720: // The raise_above_anti_dependences method returns the updated LCA and ensures
> 721: // there are no anti-dependent stores between the load's early block and the
Maybe to be explicit:
Suggestion:
// The raise_above_anti_dependences method returns the updated LCA and ensures
// there are no anti-dependent stores in any block between the load's early block and the
src/hotspot/share/opto/gcm.cpp line 724:
> 722: // updated LCA. Any stores in the updated LCA will have new anti-dependence
> 723: // edges back to the load. The caller may schedule the load in the LCA, or it
> 724: // may hoist the load above the LCA, if it is not the early block.
Suggestion:
// may hoist the load above the LCA, if the updated LCA is not the early block.
src/hotspot/share/opto/gcm.cpp line 725:
> 723: // edges back to the load. The caller may schedule the load in the LCA, or it
> 724: // may hoist the load above the LCA, if it is not the early block.
> 725: Block* PhaseCFG::raise_above_anti_dependences(Block* LCA, Node* load, bool verify) {
Could not hurt:
Suggestion:
Block* PhaseCFG::raise_above_anti_dependences(Block* LCA, Node* load, const bool verify) {
src/hotspot/share/opto/gcm.cpp line 758:
> 756: node_idx_t load_index = load->_idx;
> 757:
> 758: // Note the earliest legal placement of 'load', as determined by
Note = get?
Suggestion:
// Get the earliest legal placement of 'load', as determined by
src/hotspot/share/opto/gcm.cpp line 760:
> 758: // Note the earliest legal placement of 'load', as determined by
> 759: // the unique point in the dominator tree where all memory effects
> 760: // and other inputs are first available. (Computed by schedule_early.)
Suggestion:
// and other inputs are first available (computed by schedule_early).
src/hotspot/share/opto/gcm.cpp line 762:
> 760: // and other inputs are first available. (Computed by schedule_early.)
> 761: // For normal loads, 'early' is the shallowest place (dom graph wise)
> 762: // to look for anti-deps between this load and any store.
Just noticed when reading through the method. Cannot suggest since it's hidden:
L766-768:
- different than the schedule_early block in that it could be -> different from the schedule_early block when it is
- anti-dependences -> anti-dependencies.
src/hotspot/share/opto/gcm.cpp line 780:
> 778: ResourceArea* area = Thread::current()->resource_area();
> 779:
> 780: // Bookkeeping of possibly anti-dependent stores that we find outside of the
Suggestion:
// Bookkeeping of possibly anti-dependent stores that we find outside the
src/hotspot/share/opto/gcm.cpp line 797:
> 795: // The input load uses some memory state (initial_mem).
> 796: Node* initial_mem = load->in(MemNode::Memory);
> 797: // To find anti-dependences we must look for users of the same memory state.
Suggestion:
// To find anti-dependencies, we must look for users of the same memory state.
src/hotspot/share/opto/gcm.cpp line 854:
> 852: // - just past a MergeMem with the edge (MergeMem, use_mem_state).
> 853: assert(def_mem_state == nullptr || def_mem_state == initial_mem ||
> 854: def_mem_state->is_MergeMem(),
Suggestion:
assert(def_mem_state == nullptr || def_mem_state == initial_mem ||
def_mem_state->is_MergeMem(),
src/hotspot/share/opto/gcm.cpp line 857:
> 855: "unexpected memory state");
> 856:
> 857: uint op = use_mem_state->Opcode();
For good measure:
Suggestion:
const uint op = use_mem_state->Opcode();
src/hotspot/share/opto/gcm.cpp line 894:
> 892:
> 893: assert(!use_mem_state->is_MergeMem(),
> 894: "use_mem_state should be either a store or a memory Phi");
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 964:
> 962: if (use_mem_state->is_Phi()) {
> 963: // We have reached a memory Phi node. On our search from initial_mem to
> 964: // the Phi, we have found no anti-dependences (otherwise, we would have
Suggestion:
// the Phi, we have found no anti-dependencies (otherwise, we would have
src/hotspot/share/opto/gcm.cpp line 973:
> 971: // |??
> 972: // |||
> 973: // Phi
How about:
Suggestion:
// def_mem_state
// |
// | ? ?
// \ | /
// Phi
src/hotspot/share/opto/gcm.cpp line 1026:
> 1024: } else if (use_mem_state_block != early) {
> 1025: // We found an anti-dependent store outside the load's 'early' block.
> 1026: // The store may be between the current LCA and earliest possible block
Suggestion:
// The store may be between the current LCA and the earliest possible block
src/hotspot/share/opto/gcm.cpp line 1054:
> 1052: }
> 1053: }
> 1054: // Worklist is now empty; we have visited all possible anti-dependences.
Suggestion:
// Worklist is now empty; we have visited all possible anti-dependencies.
src/hotspot/share/opto/gcm.cpp line 1058:
> 1056: // Finished if 'load' must be scheduled in its 'early' block.
> 1057: // If we found any stores there, they have already been given
> 1058: // precedence edges.
Might be clearer since we always talked about anti-dependency edges while the concept to implement them are precedence edges.
Suggestion:
// anti-dependency edges.
src/hotspot/share/opto/gcm.cpp line 1087:
> 1085: // load from sinking past any block containing a store that may overwrite
> 1086: // memory that the load must witness.
> 1087:
Suggestion:
//
// The raised LCA will be a lower bound for placing the load, preventing the
// load from sinking past any block containing a store that may overwrite
// memory that the load must witness.
//
-------------
PR Review: https://git.openjdk.org/jdk/pull/24926#pullrequestreview-2832880054
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2084468882
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2084471291
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2084473750
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2084474619
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2084481473
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2084486896
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2084494420
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2084496026
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2084496602
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2084585843
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2084586372
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2084595136
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2084587574
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2084599234
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2084606423
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2084608271
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2084612824
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2084620462
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2084622731
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2084628087
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2084629817
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2084630749
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2084634773
More information about the hotspot-compiler-dev
mailing list