RFR: 8351280: Mark Assertion Predicates useless instead of replacing them by a constant directly

[Christian Hagedorn]

On Fri, 7 Mar 2025 08:01:13 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

> This patch is a preparatory patch for fixing https://github.com/openjdk/jdk/pull/23823 (already out for review) without relying on predicate matching during IGVN (currently proposed matching with IGVN but after looking into it and further discussions with @rwestrel, we think it's best to do it outside IGVN).
> 
> ### Update Assertion Predicate Killing Mechanism
> The main contribution of this patch is to update the killing mechanism of Assertion Predicates. Currently, we kill an Assertion Predicate by replacing its `Opaque*AssertionPredicate` node with `ConI [int:1]`. This creates problems in our predicate matching code (see for example, [JDK-8350637](https://bugs.openjdk.org/browse/JDK-8350637)). To fix this and prepare for https://github.com/openjdk/jdk/pull/23823, we move to a different approach. 
> 
> #### Mark Opaque*AssertionPredicate` Nodes Useless
> Instead of directly inserting constants, we mark `Opaque*AssertionPredicate` nodes useless when we find that an Assertion Predicate should be removed. In the next IGVN phase, we are folding it by taking the success path. This requires the following updates:
> - Introduce `_useless` flags at `Opaque*AssertionPredicate` nodes and associated mark methods.
>   - Check the flags in `Value()` and return `TypeInt::ONE` if we find them useless.
> - Adding `cmp()` method for `OpaqueInitializedAssertionPredicate` to avoid commoning up a useless with a useful node.
> - `OpaqueTemplateAssertionPredicate` nodes are by design unique to the Template Assertion Predicate because they have non-hashable `OpaqueLoop*` nodes on their input paths. I still added `hash()` that returns `NO_HASH` as an additional guarantee/contract.
> 
> #### Update Predicate Iteration Code
> To make this new approach work, we need to update the predicate iteration code. We still match Assertion Predicates with an `OpaqueAssertion*Node` to skip over them but we skip to call the `visit()` method since there should not be any work to be done for killed Assertion Predicates. This includes the additional change:
> - Refactoring `RegularPredicateBlockIterator::for_each()` which became bigger. I extracted some methods and decided to use a field instead of a local variable. This meant that I also had to remove `const` from the `for_each()` methods which I think is fine for a method that does iterations and needs to do some book keeping.
> 
> #### Other Updates
> I've also applied some small refactorings of touched code.
> 
> Thanks,
> Christian

src/hotspot/share/opto/opaquenode.cpp line 115:

> 113:     // into the bool input of an If node and can thus be replaced by true to let the Template Assertion Predicate be
> 114:     // folded away (the success path is always the true path by design).
> 115:     return phase->intcon(1);

Moved to `Value()`. There is also no guarantee that this is an existing node (though `ConI #1` should probably always exist at that point).

src/hotspot/share/opto/opaquenode.cpp line 157:

> 155: 
> 156: #ifndef PRODUCT
> 157: void OpaqueInitializedAssertionPredicateNode::dump_spec(outputStream* st) const {

Added dump for better debugging, also done for `OpaqueTemplateAssertionPredicateNode`.

src/hotspot/share/opto/opaquenode.hpp line 173:

> 171: 
> 172:   virtual int Opcode() const;
> 173:   virtual uint size_of() const { return sizeof(*this); }

Required since we add a first field to the class.

src/hotspot/share/opto/predicates.cpp line 228:

> 226:   OpaqueTemplateAssertionPredicateNode* opaque_node = this->opaque_node();
> 227:   opaque_node->mark_useless();
> 228:   igvn._worklist.push(opaque_node);

Mark instead of replacement by constant directly.

src/hotspot/share/opto/predicates.cpp line 1113:

> 1111:   if (initialized_assertion_predicate.is_last_value()) {
> 1112:     // Only Last Value Initialized Assertion Predicates need to be killed and updated.
> 1113:     initialized_assertion_predicate.kill(_phase->igvn());

Needed to move this method to the source file because of incomplete `PhaseIdealLoop` type when trying to access `igvn()`.

src/hotspot/share/opto/predicates.hpp line 692:

> 690:       if (process_initialized_assertion_predicate(predicate_visitor)) {
> 691:         continue;
> 692:       }

Refactoring: Split into methods and each method check if the opaque node is marked useless before visiting.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23941#discussion_r1984618146
PR Review Comment: https://git.openjdk.org/jdk/pull/23941#discussion_r1984620892
PR Review Comment: https://git.openjdk.org/jdk/pull/23941#discussion_r1984621379
PR Review Comment: https://git.openjdk.org/jdk/pull/23941#discussion_r1984622086
PR Review Comment: https://git.openjdk.org/jdk/pull/23941#discussion_r1984622636
PR Review Comment: https://git.openjdk.org/jdk/pull/23941#discussion_r1984623991