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

[Emanuel Peter]

On Mon, 4 Mar 2024 08:00:04 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> 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.

Could you add asserts that the `_orig_to_new` is empty before we clone? That would be a check that nothing was cloned yet, and we do not accidentally mix up two clone operations.

>> 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

Got it, great. I just looked for a matching `is_Phi` in your diff and did not find it. But it is already covered in existing code, great!

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

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