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

[Daniel Lundén]

On Wed, 7 May 2025 14:33:16 GMT, Roberto Castañeda Lozano <rcastanedalo at openjdk.org> wrote:

>> Daniel Lundén has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Updates after reviews
>
> 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?

Thanks, poor choice of wording here. What I mean is that we may call `raise_LCA_above_marks`, but the result could still be an unchanged LCA. I just simplified this description now and do not mention "attempt". Come to think of it, a less ambiguous name for this variable is `must_raise_LCA_above_marks`. I'll update it.

> 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.

Yes, good point. Updated.

> 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?

Oops, must be a leftover from an edit. Removed, thanks.

> 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");

Thanks, fixed

> 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");

Thanks, fixed

> 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

Thanks, fixed

> 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

Thanks, fixed

> 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) */ {

Thanks, fixed

> 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.

Thanks, fixed

> 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.

I agree, better to not change more than necessary. I've reverted the change.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2079949106
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2079950578
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2079951174
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2079951703
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2079951928
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2079952140
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2079952361
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2079952544
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2079952672
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2079953516