RFR: 8327109: Refactor data graph cloning used in create_new_if_for_predicate() into separate class [v3]

[Emanuel Peter]

On Mon, 4 Mar 2024 09:24:06 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> In the review process of https://github.com/openjdk/jdk/pull/16877, we identified an existing issue in `create_bool_from_template_assertion_predicate()` which is also still present in the refactoring of https://github.com/openjdk/jdk/pull/16877: In rare cases, we could endlessly re-process nodes in the DFS walk since a visited set is missing. This needs to be addressed.
>> 
>> #### Redo refactoring of `create_bool_from_template_assertion_predicate()`
>> On top of that bug, the refactored version of https://github.com/openjdk/jdk/pull/16877 is still quite complicated to understand since it tries to do multiple steps simultaneously. We've decided to redo the refactoring and better separate the steps to simplify the algorithm. By doing so, we also want to fix the existing bug. This work is split into three separate RFEs (JDK-8327109, JDK-8327110, and JDK-8327111).
>> 
>> #### Share data graph cloning code - start from existing code 
>> This first PR starts with the existing code found in `clone_nodes_with_same_ctrl()` which is called by `create_new_if_for_predicate()`. `clone_nodes_with_same_ctrl()` already does the data graph cloning in 3 separate steps which can be used as foundation:
>> 
>> 1. Collect data nodes to clone by using a node filter
>> 2. Clone the collected nodes (their data and control inputs still point to the old nodes)
>> 3. Fix the cloned data node inputs pointing to the old nodes to the cloned inputs by using an old->new mapping. In this pass, also fix the control inputs of any pinned data node from the old uncommon projection to the new one.
>> 
>> #### Shared data graph cloning class
>> Some of these steps above are shared with the data graph cloning done in `create_bool_from_template_assertion_predicate()` (refactored in JDK-8327110 and JDK-8327111). We therefore extract them in this patch such that we can reuse it in the refactoring for `create_bool_from_template_assertion_predicate()` later. We create a new `DataNodeGraph` class which does the following (to be shared) cloning of a data graph:
>> 
>> 1. Take a collection of data nodes (the collection step is different in `clone_nodes_with_same_ctrl()` compared to `create_bool_from_template_assertion_predicate()` and thus cannot be shared) and clone them. [Same as step 2 above]
>>  2. Fix the cloned data node inputs pointing to the old nodes to the cloned inputs by using an old->new mapping. [Same as first part of step 3 above but drop the second part of rewiring control inputs which is specific to ...
>
> Christian Hagedorn has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Remove dead declaration

Nice, looks better already :)

src/hotspot/share/opto/loopnode.hpp line 38:

> 36: class BaseCountedLoopEndNode;
> 37: class CountedLoopNode;
> 38: class DataInputGraph;

Suggestion:

src/hotspot/share/opto/loopopts.cpp line 4500:

> 4498: 
> 4499: // Clone all nodes in _data_nodes.
> 4500: void DataNodeGraph::clone_nodes(Node* new_ctrl) {

Suggestion:

void DataNodeGraph::clone_data_nodes(Node* new_ctrl) {

Then the comment would be obsolete

src/hotspot/share/opto/replacednodes.cpp line 211:

> 209:     }
> 210:     // Map from current node to cloned/replaced node
> 211:     OrigToNewHashtable clones(hash_table_size, hash_table_size);

Nice.
Not your problem here. But should there not be a ResouceMark before this hashtable? There is one at the beginning of the function, but we create many of these hashtables in a loop, without any ResourceMarks in between reclaiming the memory...
A but then the hashmaps and stack/to_fix etc would allocate from the ResourceArea, but start at different ResourceMarks... bad idea. Hmm.

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

Changes requested by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18080#pullrequestreview-1913808738
PR Review Comment: https://git.openjdk.org/jdk/pull/18080#discussion_r1510842256
PR Review Comment: https://git.openjdk.org/jdk/pull/18080#discussion_r1510845281
PR Review Comment: https://git.openjdk.org/jdk/pull/18080#discussion_r1510859503