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 11:59:57 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
Nice work! I'm sending out a first batch of comments before lunch. Will continue through more later.
src/hotspot/share/opto/loopPredicate.cpp line 401:
> 399: TemplateAssertionPredicateBool template_assertion_predicate_bool(opaque4_node->in(1));
> 400: BoolNode* bol = template_assertion_predicate_bool.clone(parse_predicate_proj, this);
> 401: opaque4_node = clone_and_register(opaque4_node, parse_predicate_proj);
Why do you now clone the opaque node here, and not as part of `template_assertion_predicate_bool.clone`?
src/hotspot/share/opto/loopTransform.cpp line 1390:
> 1388: // Is 'n' a node that can be found on the input chain of a Template Assertion Predicate bool (i.e. between a Template
> 1389: // Assertion Predicate If node and the OpaqueLoop* nodes)?
> 1390: static bool is_part_of_template_assertion_predicate_bool(Node* n) {
Yeah, this name was not very good, "expression" would have been better than "bool".
src/hotspot/share/opto/loopTransform.cpp line 1462:
> 1460: }
> 1461: opaque4_node = clone_and_register(opaque4_node, control);
> 1462: _igvn.replace_input_of(opaque4_node, 1, new_bool);
Again, why do we need to separately clone the `opaque4_node`?
And would it not be better to unify the two clone methods, and just pass the `nullptr`, which means clone, and non-nullptr means replace?
src/hotspot/share/opto/predicates.cpp line 180:
> 178: // Let node s be the next node being visited after node n in the DFS traversal. The following holds:
> 179: // n->in(i) = s
> 180: class DFSNodeStack : public StackObj {
It could be nice to generalize this to a `DFSInputIterator`, which takes some filter function (either as lambda / functional or as a template argument for better inlining).
I suspect we would use this in quite a few other places in the code, and this could reduce code duplication in the future, and make code much easier to read.
src/hotspot/share/opto/predicates.cpp line 182:
> 180: class DFSNodeStack : public StackObj {
> 181: Node_Stack _stack;
> 182: static const uint _no_inputs_visited_yet = 0;
Suggestion:
static const uint NO_INPUTS_VISITED_YET = 0;
src/hotspot/share/opto/predicates.cpp line 205:
> 203: }
> 204: }
> 205: return false;
What if we iterate over a whole list of inputs, and none `could_be_part`? Then we only advance the `index` by one inside `increment_top_node_input_index`. Would it not be better to simply read the `index` at the beginning, and set it just before `return false;`?
src/hotspot/share/opto/predicates.cpp line 228:
> 226: void increment_top_node_input_index() {
> 227: _stack.set_index(_stack.index() + 1);
> 228: }
could be private
src/hotspot/share/opto/predicates.cpp line 237:
> 235: // Interface to transform OpaqueLoop* nodes of a Template Assertion Predicate Bool. The transformations must return a
> 236: // new or different existing node.
> 237: class TransformOpaqueLoopNodes : public StackObj {
Suggestion:
class TransformStrategyForOpaqueLoopNodes : public StackObj {
I would like to have the Strategy in it.
And then you could rename:
`CloneOpaqueLoopNodes` -> `CloneInitAndStride(Transform)StrategyForOpaqueLoopNodes`
`CloneWithNewInit` -> `ReplaceInitAndCloneStride(Transform)StrategyForOpaqueLoopNodes`
`ReplaceOpaqueLoopNodes` -> `ReplaceInitAndStride(Transform)StrategyForOpaqueLoopNodes`
Yes, the names are longer, but it would be much clearer what they are for, in the places where they are used.
I was struggling to read `TemplateAssertionPredicateBool::clone`, and had to dig inside `clone_opaque_loop_nodes` to understand.
src/hotspot/share/opto/predicates.cpp line 349:
> 347: _index_before_cloning(phase->C->unique()),
> 348: _ctrl_for_clones(ctrl_for_clones)
> 349: DEBUG_ONLY(COMMA _found_init(false)) {}
Detail, but I would prefer if the constructor came right after the member fields. It would help me know how things are initialized quicker.
src/hotspot/share/opto/predicates.cpp line 401:
> 399:
> 400: // The transformations of this class clone the existing OpaqueLoop* nodes without any other update.
> 401: class CloneOpaqueLoopNodes : public TransformOpaqueLoopNodes {
Would it not be much simpler, if you just had one such Transform class, which can have both a new init and stride node as input, but if they are nullptr, then we just clone instead? It would also simplify the code in `clone_assertion_predicate_and_initialize`.
src/hotspot/share/opto/predicates.hpp line 270:
> 268: // A Template Assertion Predicate Bool represents the BoolNode for the initial value or the last value of a
> 269: // Template Assertion Predicate and all the nodes up to and including the OpaqueLoop* nodes.
> 270: class TemplateAssertionPredicateBool : public StackObj {
The current name targets the "bool" node, and it is not clear that you actually mean to represent the whole assertion-predicate expression. Hence:
`TemplateAssertionPredicateBool` -> `TemplateAssertionPredicateExpression`
Then `could_be_part` makes more sense. Maybe still, you could rename it to `maybe_contains`?
But is it really only "maybe" or "could"? Maybe add some explanation in what cases it looks like it could be part of it, but is actually not?
Also, expression would help to make more sense of clone, since it clones the whole expression!
-------------
Changes requested by epeter (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/16877#pullrequestreview-1794251755
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1434968527
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1434988447
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1434993226
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1434950332
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1434945062
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1434952558
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1434958185
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1434919725
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1434910049
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1434991996
PR Review Comment: https://git.openjdk.org/jdk/pull/16877#discussion_r1434986923
More information about the hotspot-compiler-dev
mailing list