On Mon, 20 Apr 2026 14:48:10 GMT, Quan Anh Mai <[email protected]> wrote:
>> Tobias Hartmann has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Reviewer comments
>
> src/hotspot/share/ci/ciInlineKlass.hpp line 65:
>
>> 63: bool maybe_flat_in_array() const;
>> 64: bool is_always_flat_in_array() const;
>> 65: bool can_be_passed_as_fields() const;
>
> I think a description of how these values are passed in and out, or a pointer
> to an existing description is appreciated here,
I added a comment here and another more detailed one to
`InlineKlass::initialize_calling_convention`. What do you think?
> src/hotspot/share/opto/subnode.cpp line 1158:
>
>> 1156: return new
>> CmpINode(uncast_in1->as_InlineType()->get_null_marker(), phase->intcon(0));
>> 1157: }
>> 1158: if (uncast_in1->is_InlineType() || uncast_in2->is_InlineType()) {
>
> This looks pretty suspicious tbh. What if we want to fast-path the
> substitutability check in C2, and compare 2 objects by pointer before looking
> at each of their fields?
I agree. Unfortunately, it's needed to ensure that the substitutability test is
optimized after being inlined - together with the code in `CmpLNode::Ideal`.
Maybe we can remove/improve it with
[JDK-8381826](https://bugs.openjdk.org/browse/JDK-8381826)?
> src/hotspot/share/opto/type.cpp line 7333:
>
>> 7331:
>> 7332:
>> //------------------------------make-------------------------------------------
>> 7333: const TypeFunc* TypeFunc::make(ciMethod* method, bool is_call, bool
>> is_osr_compilation) {
>
> It is better to have a description for the meaning of `is_call`.
I added a comment explaining the purpose of this method and its arguments.
> src/hotspot/share/runtime/sharedRuntime.cpp line 2902:
>
>> 2900: if (!_method->is_static()) {
>> 2901: // We shouldn't scalarize 'this' in a value class constructor
>> 2902: if (InlineTypePassFieldsAsArgs && holder->is_inline_klass() &&
>> InlineKlass::cast(holder)->can_be_passed_as_fields() &&
>
> Do you think it is a good idea to reduce the sites in which these flag checks
> are necessary? In this particular instance, principally, I think
> `InlineKlass::can_be_passed_as_fields` should return `false` if
> `InlineTypePassFieldsAsArgs` is `false`, removing the need to explicitly
> check the flag here.
Actually, `InlineKlass::can_be_passed_as_fields` already returns false for
`!InlineTypePassFieldsAsArgs` so this particular check is not needed. I
reverted it (though it might actually nice for consistency with below code -
but I don't have a strong opinion on this).
> src/hotspot/share/runtime/sharedRuntime.cpp line 2918:
>
>> 2916: for (SignatureStream ss(_method->signature());
>> !ss.at_return_type(); ss.next()) {
>> 2917: BasicType bt = ss.type();
>> 2918: if (InlineTypePassFieldsAsArgs && bt == T_OBJECT) {
>
> Similarly, this can also be removed.
I added this to address a comment that @fparain had when reading this code:
> [...] a point that was bothering me was that `InlineTypePassFieldsAsArgs` is
> tested after the call to ss.as_inline_klass(), this is not obvious when
> reading the code above but the test is performed in
> vk->can_be_passed_as_fields() (this is the only thing this method tests)
> which is called after having poke the system dictionary."
The expensive part here is `ss.as_inline_klass()`, so we definitely want to
guard this by `InlineTypePassFieldsAsArgs` but since that method is also called
from other places when `!InlineTypePassFieldsAsArgs &&
InlineTypeReturnedAsFields`, we can't easily move that check into the method.
Also, as Fred mentions, it's easier to read when the check is in the caller.
What do you think?
-------------
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2339#discussion_r3115567419
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2339#discussion_r3116423684
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2339#discussion_r3116509169
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2339#discussion_r3116522811
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2339#discussion_r3116550233
