RFR: 8350578: Refactor useless Parse and Template Assertion Predicate elimination code by using a PredicateVisitor

[Vladimir Kozlov]

On Thu, 13 Mar 2025 06:40:27 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> This patch cleans the Parse and Template Assertion Predicate elimination code up. We now use a single `PredicateVisitor` and share code in a new `EliminateUselessPredicates` class which contains the code previously found in `PhaseIdealLoop::eliminate_useless_predicates()`.
>> 
>> ### Unified Logic to Clean Up Parse and Template Assertion Predicates
>> We now use the following algorithm:
>> https://github.com/openjdk/jdk/blob/5e4b6ca0ddafa80eee60690caacd257b74305d4e/src/hotspot/share/opto/predicates.cpp#L1174-L1179
>> 
>> This is different from the old algorithm where we used a single boolean state `_useless`. But that does no longer work because when we first mark Template Assertion Predicates useless, we are no longer visiting them when iterating through predicates:
>> 
>> https://github.com/openjdk/jdk/blob/a21fa463c4f8d067c18c09a072f3cdfa772aea5e/src/hotspot/share/opto/predicates.hpp#L704-L708
>> 
>> We therefore require a third state. Thus, I introduced a new tri-state `PredicateState` that provides a special `MaybeUseful` value which we can set each Predicate to.
>> 
>> #### Ignoring Useless Parse Predicates
>> While working on this patch, I've noticed that we are always visiting Parse Predicates - even when they useless. We should change that to align it with what we have for the other Predicates (changed in JDK-8351280). To make this work, we also replace the `_useless` state in `ParsePredicateNode` with a new `PredicateState`.
>> 
>> #### Sharing Code for Parse and Template Assertion Predicates
>> With all the mentioned changes in place, I could nicely share code for the elimination of Parse and Template Assertion Predicates in `EliminateUselessPredicates` by using templates. The following additional changes were required:
>> 
>> - Changing the template parameter of `_template_assertion_predicate_opaques` to the more specific `OpaqueTemplateAssertionPredicateNode` type.
>> - Adding accessor methods to get the Predicate lists from `Compile`.
>> - Updating `ParsePredicate::mark_useless()` to pass in `PhaseIterGVN`, as done for Assertion Predicates
>> 
>> Note that we still do not directly replace the useless Predicates but rather mark them useless as initiated by JDK-8351280.
>> 
>> ### Other Included Changes
>> - During the various refactoring steps, I somehow dropped the code to add newly cloned Template Assertion Predicate to the `_template_assertion_predicate_opaques` list. It was done directly in the old cloning methods. This is not relevant for correctness but could ...
>
> src/hotspot/share/opto/cfgnode.hpp line 508:
> 
>> 506:   void mark_maybe_useful();
>> 507:   bool is_useful() const;
>> 508:   void mark_useful();
> 
> Needed to move these definitions to the source file because I cannot include `predicates.hpp` here due to circular dependencies. I solved this by forward declaring `PredicateState` and moving the definitions to the source file.
> 
> Same for these methods for `OpaqueTemplateAssertionPredicate` further down.

Can you add `predicates.inline.hpp` for this?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24013#discussion_r1993897088