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

[Emanuel Peter]

On Fri, 7 Mar 2025 08:06:16 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/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.

Here you push to worklist at call-site.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23941#discussion_r1984695504