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

[Emanuel Peter]

On Fri, 28 Jun 2024 13:40:50 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

> This is the next patch for Assertion Predicates. It refactors the code to create an Initialized Assertion Predicate. Changes include:
> 
> - `clone_assertion_predicate_and_initialize()` currently does two things: Cloning a Template Assertion Predicate and creating an Initailized Assertion Predicate. I've split this method into two methods `clone_template_assertion_predicate()` and `create_initialized_assertion_predicate()`:
>   - `clone_template_assertion_predicate()`: Now only clones the template. I have not cleaned the code up further because I will soon replace the `If` node with a dedicated `TemplateAssertionPredicateNode`. 
>   - `create_initialized_assertion_predicate()`: I refactored the code for Initialized Assertion Predicate into a separate class `InitializedAssertionPredicate` which creates the complete Initialized Assertion Predicate `If` with a `HaltNode`. I could get rid of some of the arguments because they can be fetched inside the new class methods.
>   - Moved `assertion_predicate_has_loop_opaque_node()` asserts to both methods.
> - Renamed `AssertionPredicateType::Init_value` -> `AssertionPredicateType::InitValue` (same for last value).
> 
> Thanks,
> Christian

Looks generally good. I left some small suggestions.

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?

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`?

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

> 410: IfTrueNode* InitializedAssertionPredicate::create(Node* control) {
> 411:   IdealLoopTree* loop = _phase->get_loop(control);
> 412:   OpaqueInitializedAssertionPredicateNode* new_opaque_bool = create_new_bool(control);

This is confusing: I ask for a `bool` but get an `opaque`...

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`.

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?

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

> 455:   Node* frame = new ParmNode(start_node, TypeFunc::FramePtr);
> 456:   _phase->register_new_node(frame, start_node);
> 457:   Node* halt = new HaltNode(fail_proj, frame, "Assertion Predicate cannot fail");

Suggestion:

  Node* halt = new HaltNode(fail_proj, frame, "Initialized Assertion Predicate cannot fail");

I would add the extra detail, just to have more context when it fails ;)

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.

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

PR Review: https://git.openjdk.org/jdk/pull/19940#pullrequestreview-2156667318
PR Review Comment: https://git.openjdk.org/jdk/pull/19940#discussion_r1664321590
PR Review Comment: https://git.openjdk.org/jdk/pull/19940#discussion_r1664331188
PR Review Comment: https://git.openjdk.org/jdk/pull/19940#discussion_r1664356749
PR Review Comment: https://git.openjdk.org/jdk/pull/19940#discussion_r1664370579
PR Review Comment: https://git.openjdk.org/jdk/pull/19940#discussion_r1664375404
PR Review Comment: https://git.openjdk.org/jdk/pull/19940#discussion_r1664378204
PR Review Comment: https://git.openjdk.org/jdk/pull/19940#discussion_r1664345892