[lworld] RFR: 8228361: [lworld] Optimize the substitutability check in C2 [v3]

[Tobias Hartmann]

On Thu, 18 Dec 2025 14:40:14 GMT, Manuel Hässig <mhaessig at openjdk.org> wrote:

>> Tobias Hartmann has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Adjustments according to review comments
>
> src/hotspot/share/opto/callnode.cpp line 1184:
> 
>> 1182:   }
>> 1183: 
>> 1184:   // Try to replace the runtime call to the substitutability test emitted by acmp if (at least) one operand is a known type
> 
> If this would be in its own helper method, `Ideal` would stay smaller and feel less about substitutability, and you could reduce the nesting with short circuit returns. Not sure if this is worth the plumbing though.

Good point, I'll factor it out.

> src/hotspot/share/opto/inlinetypenode.cpp line 694:
> 
>> 692: 
>> 693: // Emit IR to check substitutability between 'this' (left operand) and the value object referred to by 'other' (right operand).
>> 694: // Parse-time checks guarantee that both operands have the same type. If 'other' is not an InlineTypeNode, we need to emit loads for the field values.
> 
> If both `this` and `other` have the same type and `this` is an `InlineTypeNode`, how can `other` not be an `InlineTypeNode`?

The type of the node (in C2's type system) is not coupled to the node type (i.e. the C++ type): `this` can be a `InlineTypeNode` with type `TypeInstPtr` of `ciValueKlass` but `other` can, and often will be, for example, a `LoadPNode` of the same type `TypeInstPtr` of `ciValueKlass`. For example, when `other` is loaded from an `Object` field that could contain any object. Only the parse-time checks emitted by acmp then guarantee that once we reach the substitutability check, both operands will have the same type. And during parsing we often don't even know the type of `this`, so we can't cast and scalarize `other` accordingly.

> src/hotspot/share/opto/inlinetypenode.cpp line 781:
> 
>> 779:     } else {
>> 780:       assert(ft->is_primitive_type() || !ft->as_klass()->can_be_inline_klass(), "Needs substitutability test");
>> 781:       acmp_val_guard(igvn, region, phi, ctrl, bt, BoolTest::ne, this_field, other_field);
> 
> Moving the primitive case to the top of this if chain and only having the assert case in the else branch would help readability a bit.

Not sure if I'm following, do you mean by inverting the condition of `if (this_field->is_InlineType()) {`?

> test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestAcmpEdgeCases.java line 201:
> 
>> 199:                 Float.intBitsToFloat(0xFFC00000), // negative quiet
>> 200:                 Float.intBitsToFloat(0xFF800001), // negative signaling
>> 201:                 Float.intBitsToFloat(0xFFFFFFFF),  // negative max payload
> 
> This looks like something that could be reused for many tests if it were in the testlibrary.

Yes, good point. I'll think about where to put this.

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

PR Review Comment: https://git.openjdk.org/valhalla/pull/1823#discussion_r2631569385
PR Review Comment: https://git.openjdk.org/valhalla/pull/1823#discussion_r2631542513
PR Review Comment: https://git.openjdk.org/valhalla/pull/1823#discussion_r2631560286
PR Review Comment: https://git.openjdk.org/valhalla/pull/1823#discussion_r2631567758