On Thu, 30 Apr 2026 12:47:09 GMT, Tobias Hartmann <[email protected]> wrote:
> This patch addresses the C1 parts of > [JDK-8350865](https://bugs.openjdk.org/browse/JDK-8350865): [lworld] > Follow-up work from adding support for nullable, flat, atomic arrays / > fields. Comments in-line with the changes. > > Thanks, > Tobias > > --------- > - [x] I confirm that I make this contribution in accordance with the [OpenJDK > Interim AI Policy](https://openjdk.org/legal/ai). src/hotspot/cpu/aarch64/c1_LIRAssembler_aarch64.cpp line 1579: > 1577: __ cbnz(op->value()->as_register(), skip); > 1578: __ test_null_free_array_oop(op->array()->as_register(), > op->tmp()->as_register(), *op->stub()->entry()); > 1579: __ bind(skip); Not needed anymore because we have `LIRGenerator::check_null_free_array` separately. src/hotspot/cpu/aarch64/c1_LIRAssembler_aarch64.cpp line 2355: > 2353: if (is_dest) { > 2354: __ test_null_free_array_oop(obj, tmp, *slow_path->entry()); > 2355: // TODO 8350865 Flat no longer implies null-free, so we need to > check for flat dest. Can we do better here? Will be improved by JDK-8251971, see new comment `LIRGenerator::arraycopy_helper`. src/hotspot/cpu/x86/c1_LIRAssembler_x86.cpp line 1570: > 1568: __ test_flat_array_oop(op->array()->as_register(), > op->tmp()->as_register(), *op->stub()->entry()); > 1569: if (!op->value()->is_illegal()) { > 1570: // TODO 8350865 This is also used for profiling code, right? And in > that case we don't care about null but just want to know if the array is flat > or not. This is not used for profiling. We always call into the runtime for unknown flat arrays (and profile there) and if we know statically that an array is flat, we don't profile at all. src/hotspot/cpu/x86/c1_LIRAssembler_x86.cpp line 1577: > 1575: __ jcc(Assembler::notEqual, skip); > 1576: __ test_null_free_array_oop(op->array()->as_register(), > op->tmp()->as_register(), *op->stub()->entry()); > 1577: __ bind(skip); Not needed anymore because we have `LIRGenerator::check_null_free_array` separately. src/hotspot/cpu/x86/c1_LIRAssembler_x86.cpp line 2513: > 2511: if (is_dest) { > 2512: __ test_null_free_array_oop(obj, tmp, *slow_path->entry()); > 2513: // TODO 8350865 Flat no longer implies null-free, so we need to > check for flat dest. Can we do better here? Will be improved by JDK-8251971, see new comment `LIRGenerator::arraycopy_helper`. src/hotspot/share/c1/c1_Instruction.cpp line 167: > 165: if (type != nullptr) { > 166: if (type->is_loaded() && type->is_array_klass() && > type->as_array_klass()->is_refined()) { > 167: return type->as_array_klass()->is_elem_null_free(); If the type is refined, we can check statically. src/hotspot/share/c1/c1_Instruction.cpp line 293: > 291: > 292: ciType* NewObjectArray::exact_type() const { > 293: return ciObjArrayKlass::make(klass()); Now using the refined type to propagate null-free/atomic/flat properties. src/hotspot/share/c1/c1_Instruction.hpp line 1005: > 1003: assert(_buffer->as_NewInstance() != nullptr, "LoadIndexed flat > array buffer must stay a NewInstance"); > 1004: } > 1005: } Not sure how we missed this before ... src/hotspot/share/c1/c1_LIRGenerator.cpp line 1667: > 1665: if (!is_constant_null) { > 1666: LabelObj* L_isNull = new LabelObj(); > 1667: bool needs_null_check = !value.is_constant() || > value.value()->is_null_obj(); This part of the expression is always false due to the `!is_constant_null` check above. src/hotspot/share/c1/c1_LIRGenerator.cpp line 1695: > 1693: > 1694: // TODO 8350865 Can we find another way to pass an address to > access_load_at()? > 1695: class TempResolvedAddress: public Instruction { I experimented a bit with different ways of implementing this but it gets ugly pretty quickly. I decided to keep this but rename. src/hotspot/share/c1/c1_LIRGenerator.cpp line 1788: > 1786: LIRItem elm_item(elm_resolved_addr, this); > 1787: DecoratorSet decorators = IN_HEAP; > 1788: if (is_load) { Analogous to `LIRGenerator::do_LoadField`. src/hotspot/share/c1/c1_LIRGenerator.cpp line 1794: > 1792: // Null check is performed in the caller > 1793: } else { > 1794: // Zero the payload Analogous to `LIRGenerator::do_StoreField`. ------------- PR Review Comment: https://git.openjdk.org/valhalla/pull/2380#discussion_r3168166110 PR Review Comment: https://git.openjdk.org/valhalla/pull/2380#discussion_r3168020633 PR Review Comment: https://git.openjdk.org/valhalla/pull/2380#discussion_r3168207212 PR Review Comment: https://git.openjdk.org/valhalla/pull/2380#discussion_r3168200716 PR Review Comment: https://git.openjdk.org/valhalla/pull/2380#discussion_r3168021751 PR Review Comment: https://git.openjdk.org/valhalla/pull/2380#discussion_r3168212691 PR Review Comment: https://git.openjdk.org/valhalla/pull/2380#discussion_r3168215643 PR Review Comment: https://git.openjdk.org/valhalla/pull/2380#discussion_r3168218246 PR Review Comment: https://git.openjdk.org/valhalla/pull/2380#discussion_r3179480656 PR Review Comment: https://git.openjdk.org/valhalla/pull/2380#discussion_r3168242396 PR Review Comment: https://git.openjdk.org/valhalla/pull/2380#discussion_r3168228360 PR Review Comment: https://git.openjdk.org/valhalla/pull/2380#discussion_r3168227041
