RFR: 8333334: C2: Make result of `Node::dominates` more precise to enhance scalar replacement [v7]

[Tobias Hartmann]

On Tue, 9 Jul 2024 03:10:55 GMT, Qizheng Xing <qxing at openjdk.org> wrote:

>> This patch changes the algorithm of `Node::dominates` to make the result more precise, and allows the iterators of `ConcurrentHashMap` to be scalar replaced.
>> 
>> The previous algorithm will return a conservative result when encountering a dead control flow, and only try the first two input paths of a multi-input Region node, which may prevent the scalar replacement in some cases.
>> 
>> For example, with G1 GC enabled, C2 generates GC barriers for `ConcurrentHashMap` iteration operations at some early phases, and then eliminates them in a later IGVN, but `LoadNode` is also idealized in the same IGVN. This causes `LoadNode::Ideal` to see some dead barrier control flows, and refuse to split some instance field loads through Phi due to the conservative result of `Node::dominates`, and thus the scalar replacement can not be applied to iterators in the later macro elimination phase.
>> 
>> This patch allows `Node::dominates` to try other paths of the last multi-input Region node when the first path is dead, and makes `ConcurrentHashMap` iteration ~30% faster:
>> 
>> 
>> Benchmark                            (nkeys)  Mode  Cnt        Score       Error  Units
>> Maps.testConcurrentHashMapIterators    10000  avgt   15   414099.085 ± 33230.945  ns/op   # baseline
>> Maps.testConcurrentHashMapIterators    10000  avgt   15   315490.281 ±  3037.056  ns/op   # patch
>> 
>> 
>> Testing: tier1-4.
>
> Qizheng Xing has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Add copyright.

This looks good to me but you need a second review.

Please add brackets around if/else branches that you modified.

Marked as reviewed by thartmann (Reviewer).

src/hotspot/share/opto/memnode.cpp line 1664:

> 1662:   Node* region;
> 1663:   DomResult dom_result = DomResult::Dominate;
> 1664:   PhaseIterGVN* igvn = phase->is_IterGVN();

`igvn` should be moved below to its only usage.

src/hotspot/share/opto/memnode.cpp line 1687:

> 1685:         region = mem->in(0);
> 1686:       }
> 1687:       // Otherwise we encounter a complex graph.

Suggestion:

      // Otherwise we encountered a complex graph.

src/hotspot/share/opto/node.hpp line 1113:

> 1111:     NotDominate,         // 'this' node does not dominate 'sub'.
> 1112:     Dominate,            // 'this' node dominates or is equal to 'sub'.
> 1113:     EncounteredDeadCode, // Result is undefined due to encountering dead code.

Suggestion:

    EncounteredDeadCode  // Result is undefined due to encountering dead code.

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

PR Review: https://git.openjdk.org/jdk/pull/19496#pullrequestreview-2244668452
PR Review: https://git.openjdk.org/jdk/pull/19496#pullrequestreview-2244684033
PR Review Comment: https://git.openjdk.org/jdk/pull/19496#discussion_r1721251431
PR Review Comment: https://git.openjdk.org/jdk/pull/19496#discussion_r1721248449
PR Review Comment: https://git.openjdk.org/jdk/pull/19496#discussion_r1721246810