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

[Christian Hagedorn]

On Fri, 1 Mar 2024 14:09:58 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> src/hotspot/share/opto/loopPredicate.cpp line 254:
>> 
>>> 252:   const Unique_Node_List nodes_with_same_ctrl = find_nodes_with_same_ctrl(start_node, old_uncommon_proj);
>>> 253:   DataNodeGraph data_node_graph(nodes_with_same_ctrl, this);
>>> 254:   auto& orig_to_new = data_node_graph.clone(new_uncommon_proj);
>> 
>> This was a bit confusing. At first I thought you are cloning the `data_node_graph`, since the `auto` did not tell me that here we are getting a hash-table back.
>> I wonder if this cloning should be done in the constructor of `DataNodeGraph`.
>
> The beauty of packing it into the constructor is that you have fewer lines here. And that is probably beneficial if you are going to use the class elsewhere -> less code duplication.

Generally, I think there is this debate about how much work one should do in the constructor (minimal vs. maximal) and I guess there is no clear consensus. In the compiler code, we seem to tend more towards doing the work in the constructor. I agree that packing it all together to hide it from the user is quite nice. 

However, in this case here, `DataNodeGraph` is later extended (when refactoring `create_bool_from_template_assertion_predicate()` in JDK-8327110/8327111) to not only clone but also clone+transform opaque loop nodes (offering an additional method). This was the main reason I went with a separation here.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18080#discussion_r1510673858