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

[Daniel Lundén]

On Wed, 7 May 2025 14:29:07 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 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".

Sure! I went with

// Enforce a scheduling of the argument load that ensures anti-dependent stores
// do not overwrite the load's input memory state before the load executes.

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

Now simplified, does it look better? What I meant is that we do not need to raise the LCA because it is already high enough.

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

Good point, I applied your suggestion as is. This is an old comment, not sure why it was formulated in this way.

> 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

Thanks, fixed

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2079929902
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2079931275
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2079933910
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2079934375