RFR: 8335257: Refactor code to create Initialized Assertion Predicates into separate class [v2]

[Christian Hagedorn]

On Wed, 3 Jul 2024 14:46:58 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Christian Hagedorn has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
>> 
>>  - review Emanuel
>>  - Merge branch 'refs/heads/master' into JDK-8335257
>>  - 8335257: Refactor code to create Initialized Assertion Predicates into separate class
>
> src/hotspot/share/opto/loopTransform.cpp line 1505:
> 
>> 1503:   Node* frame = new ParmNode(C->start(), TypeFunc::FramePtr);
>> 1504:   register_new_node(frame, C->start());
>> 1505:   Node* halt = new HaltNode(other_proj, frame, "Template Assertion Predicate should never be executed");
> 
> The comments suggests that the predicate should never be executed. I guess it should always constant-fold? But if we hit the Halt node, we are more worried that the assertion predicate was `false`, and less worried that the predicate itself was executed, right?

The Template Assertion Predicates are removed in `eliminate_useless_predicates()` in the next loop opts run once the Parse Predicates are removed (see "PredicatesOff"). They should therefore not appear in the final graph. We still need a `HaltNode` to have a sane graph state. Therefore, I changed this message accordingly. Maybe I can update the message to make it more explicit. Note that Initialized Assertion Predicates are the ones we finally emit with debug builds and should be worried about if they are evaluated to false at runtime.

> src/hotspot/share/opto/loopTransform.cpp line 2050:
> 
>> 2048:       // Initialize from Template Assertion Predicate.
>> 2049:       assert(assertion_predicate_has_loop_opaque_node(iff), "must find OpaqueLoop* nodes");
>> 2050:       input_proj = clone_assertion_predicate_and_initialize(iff, init, stride, next_regular_predicate_proj, uncommon_proj, control,
> 
> Could you remove the `control` variable now, and just use `input_proj`?

Yes, definitely, good catch!

> src/hotspot/share/opto/predicates.cpp line 427:
> 
>> 425:       = new OpaqueInitializedAssertionPredicateNode(new_opaque->in(1)->as_Bool(), _phase->C);
>> 426:   _phase->register_new_node(opaque_bool, control);
>> 427:   return opaque_bool;
> 
> Suggestion:
> 
>   Opaque4Node* template_opaque = _template_assertion_predicate->in(1)->as_Opaque4();
>   TemplateAssertionPredicateExpression expression(template_opaque);
>   Opaque4Node* tmp_opaque = expression.clone_and_replace_init_and_stride(_new_init, _new_stride, control, _phase);
>   BoolNode* new_bool = tmp_opaque->in(1)->as_Bool();
>   OpaqueInitializedAssertionPredicateNode* new_opaque = new OpaqueInitializedAssertionPredicateNode(new_bool, _phase->C);
>   _phase->register_new_node(new_opaque, control);
>   return new_opaque;
> 
> 
> `template_opaque_bool` -> `template_opaque` (it is not a bool!)
> 
> `template_assertion_predicate_expression` -> `expression` (clear enough given type)
> 
> `new_opaque` -> `tmp_opaque` (this is a temporary, right?)
> 
> `opaque_bool` -> `new_opaque`
> 
> Suggested name for this method: `create_initialized_assertion_predicate_expression`.

See comment below.

> src/hotspot/share/opto/predicates.cpp line 431:
> 
>> 429: 
>> 430: IfNode* InitializedAssertionPredicate::create_if_node(Node* control,
>> 431:                                                       OpaqueInitializedAssertionPredicateNode* new_opaque_bool,
> 
> I see why you call it "bool". But it does clash a bit with it actually being a "opaque". Maybe we could call it the "assertion_expression", and the type helps us know what it is?

I was not super happy with "bool", either. But I think "Assertion Expression" is a good idea!

So, we have the following naming scheme:
- *Template Assertion Predicate Expression* = `Opaque4` + expression with `OpaqueLoop*Nodes` (starting at `BoolNode`)
- *Assertion Expression* = `OpaqueInitializedAssertionPredicateNode` + expression without `OpaqueLoop*Nodes` (starting at `BoolNode`)

I guess it's easier when we just use "Assertion Expression" instead of "Initialized Assertion Predicate Expression" in the context of this class. "Assertion Expression" already sounds like the real expression. So, I guess that's okay.

I've used the "Assertion Expression" in the entire class now.

What do you think?

> src/hotspot/share/opto/predicates.hpp line 400:
> 
>> 398: class InitializedAssertionPredicate : public StackObj {
>> 399:   IfNode* const _template_assertion_predicate;
>> 400:   NOT_PRODUCT(const AssertionPredicateType _assertion_predicate_type;)
> 
> I don't see the value of this field. You might as well call `template_assertion_predicate->assertion_predicate_type()` at every use-site. There are only 2.
> The issue with having a separate field here is that one wonders if this is really the same type as the one from the predicate above.

Yes, they should have the same type. Since there is currently only one method which use this, I defined it there and removed the field for now. Eventually, we need to pass the type in since I want to have a single `TemplateAssertionPredicateNode` for the init and last value. But I can still reintroduce the field again then if needed.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19940#discussion_r1665832664
PR Review Comment: https://git.openjdk.org/jdk/pull/19940#discussion_r1666464822
PR Review Comment: https://git.openjdk.org/jdk/pull/19940#discussion_r1666487070
PR Review Comment: https://git.openjdk.org/jdk/pull/19940#discussion_r1666481566
PR Review Comment: https://git.openjdk.org/jdk/pull/19940#discussion_r1666508676