On Thu, 18 Dec 2025 15:22:26 GMT, Tobias Hartmann <[email protected]> wrote:
>> src/hotspot/share/opto/inlinetypenode.cpp line 675:
>>
>>> 673: bool InlineTypeNode::can_emit_substitutability_check(Node* other)
>>> const {
>>> 674: if (other != nullptr && other->is_InlineType() && bottom_type() !=
>>> other->bottom_type()) {
>>> 675: // Different types, this is dead code because there's a check
>>> above that guarantees this.
>>
>> Shouldn't this be an assert, then? Or at least call it a sanity check
>> instead, just so no one gets an idea and removes it when they have not
>> realized yet that the bottom type check is essential for the rest of the
>> method.
>
> Maybe I'm misunderstanding your comment but with "dead code" I meant that the
> IR will be folded but didn't fold yet. We still need the checks in the C++
> code to avoid optimizing dead code during IGVN (we would need all kinds of
> is_top checks further below).
Ah, my head was in pure C++ land and did not realize that IR would be dead.
>> 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()) {`?
I meant
```c++
if (ft->is_primitive_type()) {
acmp_val_guard();
} else if (this_type->is_InlineType()) {
...
} else {
assert()
}
But now that I wrote it down, I fail to see the benefit.
-------------
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1823#discussion_r2631640846
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1823#discussion_r2631633310
