RFR: 8333393: PhaseCFG::insert_anti_dependences can fail to raise LCAs and to add necessary anti-dependence edges [v4]

[Daniel Lundén]

On Thu, 27 Feb 2025 14:21:23 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:
>> 
>>   Fix subtle bug introduced in previous update
>
> src/hotspot/share/opto/cfgnode.cpp line 2393:
> 
>> 2391:     // non-termination.
>> 2392:     uint merge_width = 0;
>> 2393:     bool split_must_terminate = false; // Is splitting guaranteed to terminate?
> 
> Needed to read it twice to understand it. How about `split_always_terminates` instead to make it more clear?

Fine with me, now changed.

> src/hotspot/share/opto/cfgnode.cpp line 2435:
> 
>> 2433:       ResourceMark rm;
>> 2434:       VectorSet visited;
>> 2435:       Node_List worklist;
> 
> You could also use a `Unique_Node_List` instead of a `Node_List` + `VectorSet`:
> 
> Unique_Node_List worklist;
> for (uint i = 0; i < worklist.size(); i++) {
> }

Correct me if I'm wrong, but doesn't this pattern lead to higher memory consumption because we need to keep already visited nodes in the worklist? It probably doesn't matter much in practice, but I find the current approach more hygienic.

> src/hotspot/share/opto/cfgnode.cpp line 2448:
> 
>> 2446:       };
>> 2447:       split_must_terminate = true; // Assume no circularity until proven otherwise.
>> 2448:       while (split_must_terminate && worklist.size() > 0) {
> 
> Seems like you have `split_must_terminate` in this condition because the `break` in the `else` path does not exit both loops. When using a separate method, you could directly return false when finding that `split_must_terminate` is false. Then you can remove `split_must_terminate` from this exit condition.

Yes, extracting the termination check to a separate function makes it possible to simplify like you suggest. Thanks!

> src/hotspot/share/opto/cfgnode.cpp line 2469:
> 
>> 2467:         }
>> 2468:       }
>> 2469:     }
> 
> Would it make sense to extract this to a separate method `is_split_through_mergemem_terminating()` (or something similar) which returns the value for `split_must_terminate`? The `PhiNode::Ideal()` method is already extremely large.

Yes, sure. Now extracted!

> test/hotspot/jtreg/compiler/codegen/TestGCMLoadPlacement.java line 30:
> 
>> 28:  * @bug 8333393
>> 29:  * @summary Test that loads are not scheduled too late.
>> 30:  * @run main/othervm -XX:+UnlockDiagnosticVMOptions
> 
> `PerMethodTrapLimit` is a pure product flag, so you can remove `-XX:+UnlockDiagnosticVMOptions`. Same below where you don't use `Stress*` flags (better double check again with a product build to be safe :-) )

Thanks, fixed (and I will rerun tests after reviews).

> test/hotspot/jtreg/compiler/codegen/TestGCMLoadPlacement.java line 31:
> 
>> 29:  * @summary Test that loads are not scheduled too late.
>> 30:  * @run main/othervm -XX:+UnlockDiagnosticVMOptions
>> 31:  *                   -XX:CompileCommand=quiet
> 
> Is `quiet` required?

Not really, removed!

> test/hotspot/jtreg/compiler/codegen/TestGCMLoadPlacement.java line 101:
> 
>> 99: 
>> 100:         int test() {
>> 101:             for (int i = 0; i < 50; ++i)
> 
> You should add missing braces here

Fixed, thanks.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23691#discussion_r1975037150
PR Review Comment: https://git.openjdk.org/jdk/pull/23691#discussion_r1975038072
PR Review Comment: https://git.openjdk.org/jdk/pull/23691#discussion_r1975041294
PR Review Comment: https://git.openjdk.org/jdk/pull/23691#discussion_r1975037516
PR Review Comment: https://git.openjdk.org/jdk/pull/23691#discussion_r1975042933
PR Review Comment: https://git.openjdk.org/jdk/pull/23691#discussion_r1975041483
PR Review Comment: https://git.openjdk.org/jdk/pull/23691#discussion_r1975043261