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

Roberto Castañeda Lozano rcastanedalo at openjdk.org
Fri May 9 17:04:53 UTC 2025


On Fri, 9 May 2025 13:46: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:
> 
>   Revise explanation of non_early_stores

Looks good (modulo the grammar glitch), thanks!

src/hotspot/share/opto/gcm.cpp line 786:

> 784:   // therefore, stores we find may be in blocks that are on completely distinct
> 785:   // control-flow paths compared to early. However, in the end, only stores in
> 786:   // blocks dominated by early matters. The reason for bookkeeping not only

Suggestion:

  // blocks dominated by early matter. The reason for bookkeeping not only

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

Marked as reviewed by rcastanedalo (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/24926#pullrequestreview-2829090692
PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2082116605


More information about the hotspot-compiler-dev mailing list