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

[Daniel Lundén]

On Mon, 12 May 2025 12:44:48 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 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.

A note on "dependencies" and "dependences": these are plural forms of "dependency" and "dependence", respectively. From what I can tell, the terms can be used more or less interchangeably, but there are some subtle differences. I will not advocate for one or the other, but as can be seen from the method name itself, other related method names, and pre-existing source code comments, the currently chosen term is "dependence". The plural is therefore "dependences", and not "dependencies".

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

See other comment above!

> 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

See other comment above!

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2090694025
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2090695143
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2090697718