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

[Emanuel Peter]

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

Looks good, I just had some minor questions / suggestions :)

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

> 140: bool OpaqueInitializedAssertionPredicateNode::cmp(const Node &n) const {
> 141:   return _useless == n.as_OpaqueInitializedAssertionPredicate()->is_useless();
> 142: }

Ah interesting. But do we ever want to common these nodes?
If we never want to common them, you could just fix it with the hash, right?

   35 class Opaque1Node : public Node {
   36   virtual uint hash() const ;                  // { return NO_HASH; }

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

> 152:   _useless = true;
> 153:   igvn._worklist.push(this);
> 154: }

Here you directly push to worklist. And for `OpaqueTemplateAssertionPredicateNode` you seem to do it at the call-site. We should probably unify this. If you want to push inside `mark_useless`, then you should probably adjust the code for `OpaqueMultiversioning` as well.

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

PR Review: https://git.openjdk.org/jdk/pull/23941#pullrequestreview-2666631783
PR Review Comment: https://git.openjdk.org/jdk/pull/23941#discussion_r1984686387
PR Review Comment: https://git.openjdk.org/jdk/pull/23941#discussion_r1984691481