RFR: 8327109: Refactor data graph cloning used in create_new_if_for_predicate() into separate class
Christian Hagedorn
chagedorn at openjdk.org
Mon Mar 4 08:13:59 UTC 2024
On Fri, 1 Mar 2024 14:11:17 GMT, Emanuel Peter <epeter 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 ...
>
> src/hotspot/share/opto/loopPredicate.cpp line 255:
>
>> 253: DataNodeGraph data_node_graph(nodes_with_same_ctrl, this);
>> 254: auto& orig_to_new = data_node_graph.clone(new_uncommon_proj);
>> 255: fix_cloned_data_node_controls(old_uncommon_proj, new_uncommon_proj, orig_to_new);
>
> And is there a reason why `fix_cloned_data_node_controls` is not part of the `DataNodeGraph` class? Is there any use of the class where we don't have to call `fix_cloned_data_node_controls`?
The way we fix the control inputs here is very specific to this code (I don't think we'll do something similar elsewhere). `create_bool_from_template_assertion_predicate()` only does cloning of non-pinned nodes and does not need to do rewire controls - I think this code could be reused at other places as well but that could be cleaned up separately.
If we later refactor other cases which needs to rewire the control nodes in a specific way, we could still try to move the code of `fix_cloned_data_node_controls()` inside `DataNodeGraph` and try to share it.
> src/hotspot/share/opto/loopPredicate.cpp line 256:
>
>> 254: auto& orig_to_new = data_node_graph.clone(new_uncommon_proj);
>> 255: fix_cloned_data_node_controls(old_uncommon_proj, new_uncommon_proj, orig_to_new);
>> 256: Node** cloned_node_ptr = orig_to_new.get(start_node);
>
> Boah, this `**` is a bit nasty. Would have been nicer if there was a reference pass instead, which checks already that the element exists.
That's indeed not that great. Maybe the hash table class should provide an extra function to get references/pointers back (I see why returning a pointer is useful when you directly store objects instead of pointers into the hash table) - not sure though if we should squeeze that into this PR. Maybe in a separate RFE?
> src/hotspot/share/opto/loopPredicate.cpp line 265:
>
>> 263: void PhaseIdealLoop::fix_cloned_data_node_controls(
>> 264: const ProjNode* old_uncommon_proj, Node* new_uncommon_proj,
>> 265: const ResizeableResourceHashtable<Node*, Node*, AnyObj::RESOURCE_AREA, mtCompiler>& orig_to_new) {
>
> Suggestion:
>
> const ResizeableResourceHashtable<Node*, Node*, AnyObj::RESOURCE_AREA, mtCompiler>& orig_to_new)
> {
>
> This might also help with understanding the indentation. But this is a taste question for sure.
Will change with indentation fix.
> src/hotspot/share/opto/loopPredicate.cpp line 271:
>
>> 269: set_ctrl(clone, new_uncommon_proj);
>> 270: }
>> 271: });
>
> Indentation is suboptimal here. I found it difficult to read.
> Style guide:
>
>
> Indentation for multi-line lambda:
>
> c.do_entries([&] (const X& x) {
> do_something(x, a);
> do_something1(x, b);
> do_something2(x, c);
> });
Good point, I was not aware of this formatting rule. Will fix that.
> src/hotspot/share/opto/loopPredicate.cpp line 291:
>
>> 289: for (uint i = 1; i < next->req(); i++) {
>> 290: Node* in = next->in(i);
>> 291: if (!in->is_Phi()) {
>
> What happened with the `is_Phi`? Is it not needed anymore?
See later comment in `DataNodeGraph`.
> src/hotspot/share/opto/loopnode.hpp line 1921:
>
>> 1919: rewire_clones_to_cloned_inputs();
>> 1920: return _orig_to_new;
>> 1921: }
>
> Currently, it looks like one could call `clone` multiple times. But I think that would be wrong, right?
> That is why I'd put all the active logic in the constructor, and only the passive stuff is publicly accessible, with `const` to indicate that these don't have any effect.
Yes, that would be unexpected, so I agree with you here. But as mentioned earlier, we need to add another method to this class later which does the cloning slightly differently, so we cannot do all the work in the constructor.
We probably have multiple options here:
- Do nothing (could be reasonable as this class is only used rarely and if it's used it's most likely uncommon to clone twice in a row on the same object - and if one does, one probably has a look at the class anyway to notice what to do).
- Add asserts to ensure `clone()` is only called once (adds more code but could be a low overhead option - however, we should think about whether we really want to save the user from itself).
- Return a copy of the hash table and clear it afterward (seems too much overhead for having no such use-case).
I think option 1 and 2 are both fine.
> src/hotspot/share/opto/loopopts.cpp line 4519:
>
>> 4517: _orig_to_new.iterate_all([&](Node* node, Node* clone) {
>> 4518: for (uint i = 1; i < node->req(); i++) {
>> 4519: Node** cloned_input = _orig_to_new.get(node->in(i));
>
> You don't need to check for `is_Phi` on `node->in(i)` anymore?
Could have added a comment here about the `is_Phi()` drop. The `DataNodeGraph` class already takes a node collection to clone. We therefore do not need to additionally check for `is_Phi()` here. If an input is a phi, it would not have been cloned in the first place because the node collection does not contain phis (L239):
https://github.com/openjdk/jdk/blob/c00cc8ffaee9bf9b3278d84afba0af2ac00134de/src/hotspot/share/opto/loopPredicate.cpp#L231-L245
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18080#discussion_r1510681911
PR Review Comment: https://git.openjdk.org/jdk/pull/18080#discussion_r1510686211
PR Review Comment: https://git.openjdk.org/jdk/pull/18080#discussion_r1510708206
PR Review Comment: https://git.openjdk.org/jdk/pull/18080#discussion_r1510696808
PR Review Comment: https://git.openjdk.org/jdk/pull/18080#discussion_r1510699583
PR Review Comment: https://git.openjdk.org/jdk/pull/18080#discussion_r1510721511
PR Review Comment: https://git.openjdk.org/jdk/pull/18080#discussion_r1510703408
More information about the hotspot-compiler-dev
mailing list