RFR: 8351568: Improve source code documentation for PhaseCFG::insert_anti_dependences

[Daniel Lundén]

On Tue, 29 Apr 2025 04:58:39 GMT, Galder Zamarreño <galder 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.
>
> src/hotspot/share/opto/gcm.cpp line 889:
> 
>> 887:             // since the load will be forced into a block preceding the Phi.
>> 888:             pred_block->set_raise_LCA_mark(load_index);
>> 889:             assert(!LCA_orig->dominates(pred_block) ||
> 
> Has this assert moved elsewhere? Or do we really want to remove it altogether?

Thanks for bringing this up; I was meaning to write about this particular removal (and a similar removal towards the end of `insert_anti_dependences`) in the PR description, but it slipped my mind.

The first removed `assert` is the following.

assert(!LCA_orig->dominates(pred_block) ||
       early->dominates(pred_block), "early is high enough");

This `assert` is equivalent to the implication "LCA_orig dominates pred_block" ⇒ "early dominates pred_block". We can assert something much stronger, namely that early dominates LCA_orig. This, by definition, entails that "LCA_orig dominates b" ⇒ "early dominates b" for _any_ block b. I added such an assert (see below) early in `insert_anti_dependences` and removed the current assert.

assert(early->dominates(LCA_orig), "precondition failed");


The situation is analogous for the other `assert` that I removed.

assert(LCA->dominates(store_block)
       || !LCA_orig->dominates(store_block), "no stray stores");

I simply replaced it with another assert at the end if `insert_anti_dependences`:

assert(LCA->dominates(LCA_orig), "unsound updated LCA");


Furthermore, we also already have an assert in `raise_LCA_above_marks` that verifies the new LCA everytime we update it.

assert(early->dominates(LCA), "unsound LCA update");

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24926#discussion_r2068360455