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

[Manuel Hässig]

On Thu, 18 Dec 2025 15:22:26 GMT, Tobias Hartmann <thartmann at openjdk.org> 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