RFR: 8351568: Improve source code documentation for PhaseCFG::insert_anti_dependences [v6]
Daniel Lundén
dlunden at openjdk.org
Thu May 15 09:34:41 UTC 2025
On Mon, 12 May 2025 11:26:58 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:
>> 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>
>
> 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:
Thanks, fixed!
> 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?
Thanks, fixed!
> 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
LCA is actually a separate argument to the method. Rewrote it a bit now so it is hopefully clearer, have a look and see what you think.
> 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
Thanks, fixed!
> 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
Thanks, added!
> 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
Thanks, added!
> 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.
Thanks, updated. I also changed other occurrences of "the LCA" in this paragraph to "the updated LCA" for consistency.
> 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) {
Thanks, fixed!
> 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
I changed to "Record the earliest ..." instead as I think that is more clear.
> 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).
Thanks, fixed! Also made some other edits in this comment.
> 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
Thanks, fixed!
> 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(),
Thanks, fixed!
> 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();
Thanks, fixed!
> 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");
Thanks, fixed!
> src/hotspot/share/opto/gcm.cpp line 973:
>
>> 971: // |??
>> 972: // |||
>> 973: // Phi
>
> How about:
> Suggestion:
>
> // def_mem_state
> // |
> // | ? ?
> // \ | /
> // Phi
Yes, looks better, thanks (fixed).
> 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
Thanks, fixed!
> 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.
See other comment above!
> 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.
Yes, best to be consistent. Fixed (although I changed to anti-dependence, see other comment above).
> 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.
> //
Thanks, fixed!
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2090706887
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2090707290
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2090708819
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2090709124
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2090709509
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2090710421
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2090711906
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2090712169
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2090714079
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2090715205
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2090715489
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2090717606
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2090717853
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2090717990
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2090718600
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2090718878
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2090719119
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2090721321
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2090721476
More information about the hotspot-compiler-dev
mailing list