RFR: 8305638: Refactor Template Assertion Predicate Bool creation and Predicate code in Split If and Loop Unswitching
Emanuel Peter
epeter at openjdk.org
Fri Dec 22 15:48:08 UTC 2023
On Wed, 29 Nov 2023 08:42:41 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:
> This patch is intended for JDK 23.
>
> While preparing the patch for the full fix for Assertion Predicates [JDK-8288981](https://bugs.openjdk.org/browse/JDK-8288981), I still noticed that some changes are not required for the actual fix and could be split off and reviewed separately in this PR.
>
> The patch applies the following cleanup changes:
> - The complete fix had to add slightly different cloning cases in `PhaseIdealLoop::create_bool_from_template_assertion_predicate()` which already has quite some logic to switch between different cases. Additionally, the algorithm in the method itself was already hard to understand and difficult to adapt. I therefore re-implemented it in a separate class `CloneTemplateAssertionPredicateBool` together with some helper classes like `DFSNodeStack`. To use it, I've added a `TemplateAssertionPredicateBool` class that offers three cloning possibilities:
> - `clone()`: Clone without modification
> - `clone_and_replace_opaque_loop_nodes()`: Clone and replace the `OpaqueLoop*Nodes` with a new init and stride node.
> - `clone_and_replace_init()`: Special case of `clone_and_replace_opaque_loop_nodes()` which only replaces `OpaqueLoopInitNode` and clones `OpaqueLoopStrideNode`.
>
> This refactoring could be extracted from the complete fix.
> - The Split If code to detect (`subgraph_has_opaque()`) and clone Template Assertion Predicate Bools was extracted to a separate class `CloneTemplateAssertionPredicateBoolDown` and uses the new `TemplateAssertionPredicateBool` class to do the actual cloning.
> - In the process of coding the complete fix, I've refactored the Loop Unswitching code quite a bit. This change could also be extracted into a separate RFE. Changes include:
> - Renaming
> - Extracting code to separate classes/methods
> - Adding comments
> - Some small refactoring including:
> - Removing unused parameters
> - Renaming variables/parameters/methods
>
> Thanks,
> Christian
Next batch of comments. More coming.
src/hotspot/share/opto/loopUnswitch.cpp line 117:
> 115:
> 116: // Perform Loop Unswitching on the loop containing an invariant test that does not exit the loop. The loop is cloned
> 117: // such that we have two identical loops next to each other - a fast and a slow loop. We modify the loops as follows:
It sounds like they are somehow identical and at the same time modified.
Suggestion: say that they are "first" cloned to be identical, and then "second" modified such that one becomes the "true-path" and the other the "false-path" loop. I wonder if we can eliminate the "fast / slow" wording, because who really knows which path is faster or slower, right?
src/hotspot/share/opto/loopUnswitch.cpp line 232:
> 230: IfFalseNode* _slow_loop_proj;
> 231:
> 232: IfNode* create_selector_if(IdealLoopTree* loop, IfNode* unswitch_if_candidate) {
I'd make this `const` if possible. You call this during initialization in the constructor. It would be nice to know that it does not have side-effects.
src/hotspot/share/opto/loopUnswitch.cpp line 241:
> 239: unswitch_if_candidate->_fcnt) :
> 240: new IfNode(_original_loop_entry, unswitching_candidate_bool, unswitch_if_candidate->_prob,
> 241: unswitch_if_candidate->_fcnt);
Could be nice to have some utility method that creates either, based on opcode, right? Same pattern exists in `PhaseIdealLoop::insert_if_before_proj`.
IfNode* new_if = (opcode == Op_If) ? new IfNode(proj2, bol, iff->_prob, iff->_fcnt):
new RangeCheckNode(proj2, bol, iff->_prob, iff->_fcnt);
src/hotspot/share/opto/loopUnswitch.cpp line 256:
> 254: _phase->register_node(slow_loop_proj, _outer_loop, _selector, _dom_depth);
> 255: return slow_loop_proj;
> 256: }
Looks like code duplication. Suggestion:
`create_selector_proj(bool con)`
src/hotspot/share/opto/loopUnswitch.cpp line 266:
> 264: _selector(create_selector_if(loop, unswitch_if_candidate)),
> 265: _fast_loop_proj(create_fast_loop_proj()),
> 266: _slow_loop_proj(create_slow_loop_proj()) {}
It seems to me you could make all fields `const` of some sort, right?
src/hotspot/share/opto/loopUnswitch.cpp line 296:
> 294: IdealLoopTree* _loop;
> 295: Node_List* _old_new;
> 296: PhaseIdealLoop* _phase;
Now you add yet another term: Original
We already have Old/New, True/False, Slow/Fast.
I would call this `OldLoop`
src/hotspot/share/opto/loopUnswitch.cpp line 300:
> 298: void fix_loop_entries(IfProjNode* iffast_pred, IfProjNode* ifslow_pred) {
> 299: _phase->replace_loop_entry(_strip_mined_loop_head, iffast_pred);
> 300: LoopNode* slow_loop_strip_mined_head = _old_new->at(_strip_mined_loop_head->_idx)->as_Loop();
utility method `old_to_new` could be helpful, and eliminate this line.
src/hotspot/share/opto/loopUnswitch.cpp line 325:
> 323: _loop(loop),
> 324: _old_new(old_new),
> 325: _phase(loop->_phase) {}
Again, I think it would be nice to have the constructor and public member methods at the beginning, right after the field definition. It just helps to see how they belong together better.
src/hotspot/share/opto/loopUnswitch.cpp line 334:
> 332: const uint first_slow_loop_node_index = _phase->C->unique();
> 333: _phase->clone_loop(_loop, *_old_new, _phase->dom_depth(_loop_head),
> 334: PhaseIdealLoop::CloneIncludesStripMined, loop_selector);
I think this used to be:
`clone_loop(loop, old_new, dom_depth(head->skip_strip_mined()), mode, iff);`
Should it be `dom_depth` of `_strip_mined_loop_head`?
src/hotspot/share/opto/loopUnswitch.cpp line 337:
> 335: // Fast (true) and Slow (false) control
> 336: IfProjNode* iffast_pred = unswitched_loop_selector.fast_loop_proj();
> 337: IfProjNode* ifslow_pred = unswitched_loop_selector.slow_loop_proj();
I'm also not happy that we now have different names: `fast_loop_proj` vs `iffast_pred`. Maybe you need to replace `iffast_pred` wherever it is used? And you could take these 2 lines up to the other one that asks for the selector.
src/hotspot/share/opto/loopUnswitch.cpp line 344:
> 342: DEBUG_ONLY(verify_unswitched_loops(_loop_head, unswitched_loop_selector, _old_new);)
> 343: return loop_selector;
> 344: }
Should we return the `unswitched_loop_selector` instead? Because where it is used, we will need the projections again, right?
src/hotspot/share/opto/loopnode.cpp line 4141:
> 4139: void PhaseIdealLoop::collect_useful_template_assertion_predicates_for_loop(IdealLoopTree* loop,
> 4140: Unique_Node_List &useful_predicates) {
> 4141: Node* entry = loop->_head->as_Loop()->skip_strip_mined()->in(LoopNode::EntryControl);
looks like a bug-fix?
src/hotspot/share/opto/predicates.cpp line 152:
> 150: }
> 151:
> 152: TemplateAssertionPredicateBool::TemplateAssertionPredicateBool(Node* source_bool) : _source_bool(source_bool->as_Bool()) {
Would be cleaner to just require the input to be a `BoolNode` right?
src/hotspot/share/opto/predicates.cpp line 166:
> 164: }
> 165: }
> 166: assert(has_template_output, "must find Template Assertion Predicate as output");
Idea: pack this in a method `has_opaque4_output`. Then you do not need the `has_template_output` variable, but can directly `return true` and `return false` at the end. And call like that:
`DEBUG_ONLY(source_bool->has_opaque4_output();)`
The nice thing: the constructor could be moved to the `hpp` file.
src/hotspot/share/opto/predicates.cpp line 212:
> 210: }
> 211:
> 212: uint node_index_to_previously_visited_parent() const {
This is the "parent" of the current node, right?
Parent is also a bit confusing, I think. Maybe use "input" instead? Because at the beginning you say the DFS traverses the inputs.
Why do you say "previously visited"?
src/hotspot/share/opto/predicates.cpp line 230:
> 228: }
> 229:
> 230: void replace_top_with(Node* node) {
`replace_top_node_with`
src/hotspot/share/opto/predicates.cpp line 245:
> 243: // Class to clone a Template Assertion Predicate Bool. The BoolNode and all the nodes up to but excluding the OpaqueLoop*
> 244: // nodes are cloned. The OpaqueLoop* nodes are transformed by the provided strategy (e.g. cloned or replaced).
> 245: class CloneTemplateAssertionPredicateBool : public StackObj {
Suggestion:
class CloneTemplateAssertionPredicateExpression : public StackObj {
src/hotspot/share/opto/predicates.cpp line 250:
> 248: uint _index_before_cloning;
> 249: Node* _ctrl_for_clones;
> 250: DEBUG_ONLY(bool _found_init;)
what about stride?
src/hotspot/share/opto/predicates.hpp line 271:
> 269: // Template Assertion Predicate and all the nodes up to and including the OpaqueLoop* nodes.
> 270: class TemplateAssertionPredicateBool : public StackObj {
> 271: BoolNode* _source_bool;
could be a const pointer
-------------
Changes requested by epeter (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/16877#pullrequestreview-1794461358
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1435043499
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1435093011
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1435100821
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1435105058
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1435088483
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1435082399
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1435135799
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1435062571
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1435125509
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1435120810
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1435117695
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1435141647
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1435153913
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1435157506
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1435162747
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1435166023
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1435167410
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1435167783
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1435154277
More information about the hotspot-compiler-dev
mailing list