[lworld] RFR: [lworld] C2: Support abstract value class fields

Wed Sep 4 12:27:34 UTC 2024

On Wed, 4 Sep 2024 10:32:49 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

> This patch adds support for abstract value class fields. To make it work, it required fixes in various places. The two main things to change was to account for fields in abstract value classes when collecting/accessing all fields which was not expected before and updating the way we handle larvals.
> 
> ## Updated Larval Handling
> ### Previous Larval Initialization
> Before this change, larvals were always updated inside a single concrete value class constructor. It could have been done directly:
> 
> new MyValue -> MyValue.<init> (initialization here) -> Object.<init>
> 
> or by first delegating to another value type constructor inside the same value class which does the initiliazation:
> 
> new MyValue -> MyValue.<init> -> MyValue.<init> (initialization here) -> Object.<init>
> 
> It was not possible to distribute the initialization of an inline type to multiple constructors (i.e. at value class constructor cannot do a partial initialization).
> 
> Therefore, we could just detect any call from a value class constructor to an abstract constructor or the `Object` constructor with the same receiver and then mark the inline type as non-larval since it's completely initialized before calling the super constructor (abstract classes and `Object.<init>` cannot modify the inline type anymore). The only thing required after the call was to detect if we actually inlined another constructor with the same receiver within the same concrete value class. If that's the case, we need to update the exit map to propagate the initialized receiver to the caller.
> 
> This now changes: With fields in abstract value classes, a larval is not completely initialized after the concrete value class constructor is complete. We therefore needed to find a new way to ensure the correct initialization of larvals.
> 
> ### New Larval Initialization with Fields in Abstract Classes
> #### Problems
> - A larval _could_ be fully initialized after calling the concrete value class constructor. But we could also have a situation where we still need to initialize a field in an abstract value class. Therefore, we cannot eagerly mark an inline type as non-larval once the concrete value class constructor is completed nor when an abstract value class constructor is completed (there could be multiple).
> - The currently parsed method only has the map/JVM state from the direct caller. We therefore cannot simply detect the call to the `Object.<init>()` call at the end of chained value class constructor calls and then propagate the update to al...

Excellent work, Christian! Great analysis and proper fix. I know how tedious it was to find all these tests to trigger all construction variants and investigate the correct fix.

I just left a few code style comments.

src/hotspot/share/ci/ciInlineKlass.cpp line 38:

> 36:   GrowableArray<ciField*>* super_klass_fields = nullptr;
> 37:   if (super_klass != nullptr && super_klass->has_nonstatic_fields()) {
> 38:     int super_flen   = super_klass->nof_nonstatic_fields();

Suggestion:

    int super_flen = super_klass->nof_nonstatic_fields();

src/hotspot/share/compiler/methodLiveness.cpp line 627:

> 625:           (method->is_object_constructor() &&
> 626:            ((!holder->is_abstract() && holder->is_inlinetype()) ||
> 627:             holder->is_abstract()))) {

Suggestion:

      if (method->intrinsic_id() == vmIntrinsics::_Object_init ||
          (method->is_object_constructor() && ((!holder->is_abstract() && holder->is_inlinetype()) || holder->is_abstract()))) {

It's personal taste but I find this version more readable.

src/hotspot/share/compiler/methodLiveness.cpp line 628:

> 626:            ((!holder->is_abstract() && holder->is_inlinetype()) ||
> 627:             holder->is_abstract()))) {
> 628:         // Returning from Object.<init> implicitly registers a finalizer for the receiver if needed, so keep it alive.

Suggestion:

        // Returning from Object.<init> implicitly registers a finalizer for the receiver if needed, to keep it alive.

src/hotspot/share/compiler/methodLiveness.cpp line 631:

> 629:         // Value class constructors update the scalarized receiver. We need to keep it live so that we can find it after
> 630:         // (chained) constructor calls and propagate updates to the caller. If the holder of the constructor is abstract,
> 631:         // we do not know if the constructor was called from a value class or not. We therefore keep the receiver of all

Should this be

Suggestion:

        // we do not know if the constructor was called on a value class or not. We therefore keep the receiver of all

?

src/hotspot/share/oops/fieldStreams.hpp line 296:

> 294:   const FieldStreamType& current() const {
> 295:     return _current_stream;
> 296:   }

I would suggest to rather add `is_flat` and `index` wrapper methods instead of exposing `_current_stream`.

src/hotspot/share/oops/inlineKlass.cpp line 247:

> 245: // types is an argument: drop all T_METADATA/T_VOID from the list).
> 246: //
> 247: // Inline types could also have fields in abstract super value classes.

Suggestion:

// Value classes could also have fields in abstract super value classes.

src/hotspot/share/opto/doCall.cpp line 807:

> 805:         _caller->map()->argument(_caller, 0)->bottom_type()->is_inlinetypeptr();
> 806:     assert(!is_current_method_inline_type_constructor || !cg->method()->is_object_constructor() || receiver != nullptr,
> 807:            "must have valid receiver after calling another inline type constructor");

It's not guaranteed here (yet) that `cg()->method()` is an inline type constructor.

src/hotspot/share/opto/doCall.cpp line 807:

> 805:         _caller->map()->argument(_caller, 0)->bottom_type()->is_inlinetypeptr();
> 806:     assert(!is_current_method_inline_type_constructor || !cg->method()->is_object_constructor() || receiver != nullptr,
> 807:            "must have valid receiver after calling another inline type constructor");

Suggestion:

           "must have valid receiver after calling another constructor");

src/hotspot/share/opto/doCall.cpp line 812:

> 810:         cg->method()->is_object_constructor() && receiver->bottom_type()->is_inlinetypeptr() &&
> 811:         // AND:
> 812:         // 1) ...has the same receiver? Then it's another constructor of the same class doing the initialization.

Suggestion:

         // AND:
         // 1) ... invoked on the same receiver? Then it's another constructor on the same object doing the initialization.

src/hotspot/share/opto/doCall.cpp line 814:

> 812:         // 1) ...has the same receiver? Then it's another constructor of the same class doing the initialization.
> 813:         (receiver == _caller->map()->argument(_caller, 0) ||
> 814:          // 2) ...is abstract? Then it's the call to the super constructor which eventually calls Object.<init> to

Suggestion:

         // 2) ... abstract? Then it's the call to the super constructor which eventually calls Object.<init> to

src/hotspot/share/opto/doCall.cpp line 818:

> 816:          cg->method()->holder()->is_abstract() ||
> 817:          // 3) ...Object.<init>? Then we know it's the final call to finish the larval initialization. Other
> 818:          //       Object.<init> calls would have a non-inline-type receiver which we already excluded in the check above.

Suggestion:

         // 3) ... Object.<init>? Then we know it's the final call to finish the larval initialization. Other
         //        Object.<init> calls would have a non-inline-type receiver which we already excluded in the check above.

src/hotspot/share/opto/graphKit.cpp line 1898:

> 1896:       // Pass inline type argument via oop to callee
> 1897:       InlineTypeNode* inline_type = arg->as_InlineType();
> 1898:       const ciMethod* caller_method = _method;

Unused?

src/hotspot/share/opto/graphKit.cpp line 1901:

> 1899:       const ciMethod* method = call->method();
> 1900:       ciInstanceKlass* holder = method->holder();
> 1901:       const bool is_receiver = i == TypeFunc::Parms;

Suggestion:

      const bool is_receiver = (i == TypeFunc::Parms);

src/hotspot/share/opto/graphKit.cpp line 1915:

> 1913:         must_init_buffer = true;
> 1914:       }
> 1915:       arg = inline_type->buffer(this, true, must_init_buffer);

I think `must_init_buffer` should be true by default and set to false here for larvals, see comments below.

src/hotspot/share/opto/graphKit.cpp line 1997:

> 1995:     Node* receiver = call->in(TypeFunc::Parms);
> 1996:     if (receiver->bottom_type()->is_inlinetypeptr()) {
> 1997:       InlineTypeNode* inline_type_receiver = receiver->as_InlineType();

Suggestion:

    InlineTypeNode* inline_type_receiver = call->in(TypeFunc::Parms)->isa_InlineType();
    if (receiver != nullptr) {

src/hotspot/share/opto/inlinetypenode.cpp line 264:

> 262:   // updated. If we scalarize now and update the fields, we cannot propagate the update out of the constructor call
> 263:   // because the scalarized fields are local to this method. We need to use the buffer to make the update visible to
> 264:   // the outside (see also CompiledEntrySignature::compute_calling_conventions()).

Suggestion:

  // We should not scalarize larvals in debug info of their constructor calls because their fields could still be
  // updated. If we scalarize and update the fields in the constructor, the updates won't be visible in the caller after deoptimization
  // because the scalarized field values are local to the caller. We need to use a buffer to make the updates visible to
  // the outside.

src/hotspot/share/opto/inlinetypenode.cpp line 269:

> 267:       sfpt->in(TypeFunc::Parms) == this) {
> 268:     // Receiver of larval is always buffered in its constructor call because it was initially created outside the
> 269:     // constructor.

Suggestion:

    // Receiver is always buffered because it's passed as oop, see special case in CompiledEntrySignature::compute_calling_conventions().

src/hotspot/share/opto/inlinetypenode.cpp line 601:

> 599:       //                  create a new buffer. Once the larval escapes, we will initialize the buffer (must_init set).
> 600:       assert(!must_init || is_larval(), "must_init should only be set for larval");
> 601:       if (!is_larval() || must_init) {

This is confusing: Shouldn't the default of `must_init` be true because that's the common case? And then we can assert here that it can only ever be false if `is_larval()` is true.

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

Marked as reviewed by thartmann (Committer).

PR Review: https://git.openjdk.org/valhalla/pull/1236#pullrequestreview-2279823394
PR Review Comment: https://git.openjdk.org/valhalla/pull/1236#discussion_r1743578078
PR Review Comment: https://git.openjdk.org/valhalla/pull/1236#discussion_r1743584985
PR Review Comment: https://git.openjdk.org/valhalla/pull/1236#discussion_r1743586545
PR Review Comment: https://git.openjdk.org/valhalla/pull/1236#discussion_r1743588472
PR Review Comment: https://git.openjdk.org/valhalla/pull/1236#discussion_r1743593742
PR Review Comment: https://git.openjdk.org/valhalla/pull/1236#discussion_r1743594412
PR Review Comment: https://git.openjdk.org/valhalla/pull/1236#discussion_r1743601667
PR Review Comment: https://git.openjdk.org/valhalla/pull/1236#discussion_r1743606921
PR Review Comment: https://git.openjdk.org/valhalla/pull/1236#discussion_r1743609549
PR Review Comment: https://git.openjdk.org/valhalla/pull/1236#discussion_r1743616161
PR Review Comment: https://git.openjdk.org/valhalla/pull/1236#discussion_r1743616516
PR Review Comment: https://git.openjdk.org/valhalla/pull/1236#discussion_r1743622995
PR Review Comment: https://git.openjdk.org/valhalla/pull/1236#discussion_r1743621506
PR Review Comment: https://git.openjdk.org/valhalla/pull/1236#discussion_r1743681859
PR Review Comment: https://git.openjdk.org/valhalla/pull/1236#discussion_r1743628383
PR Review Comment: https://git.openjdk.org/valhalla/pull/1236#discussion_r1743639902
PR Review Comment: https://git.openjdk.org/valhalla/pull/1236#discussion_r1743646004
PR Review Comment: https://git.openjdk.org/valhalla/pull/1236#discussion_r1743677905