JDK-8326438 : C2: assert(ld->in(1)->Opcode() == Op_LoadN) failed: Assumption invalid: input to DecodeN is not LoadN
  • Type: Bug
  • Component: hotspot
  • Sub-Component: compiler
  • Affected Version: 23
  • Priority: P2
  • Status: Resolved
  • Resolution: Fixed
  • OS: linux
  • CPU: aarch64
  • Submitted: 2024-02-21
  • Updated: 2024-03-28
  • Resolved: 2024-03-25
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 23
23 b16Fixed
Related Reports
Relates :  
JDK-8310524 - C2: record parser-generated LoadN nodes for IGVN
Description
# Failure analysis

The assert in question, added in JDK-8310524, is too strong. It holds in the vast majority of cases, but there are exceptions due to the GVN transformation at https://github.com/openjdk/jdk/blob/8cb9b479c529c058aee50f83920db650b0c18045/src/hotspot/share/opto/memnode.cpp#L973.

The attached file `graph.png` illustrates the exceptional case identified in this issue. Two inlined method calls produce two LoadNs which merge and are then passed to two DecodeNs. The merging Phi node, separating the LoadNs and DecodeNs, triggers the assert.

The compiler proves that, in the context at the inlining location, the object for the inlined method belongs to one of two possible classes. Inlining both possible method versions (and guarding them with appropriate class checks) is cheap and therefore likely considered a good option by the compiler.


# Original description

Stack: [0x00004000d5c01000,0x00004000d5dff000],  sp=0x00004000d5df7d80,  free space=2011k
Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
V  [libjvm.so+0xc571c8]  GraphKit::make_load(Node*, Node*, Type const*, BasicType, int, MemNode::MemOrd, LoadNode::ControlDependency, bool, bool, bool, bool, unsigned char)+0x268  (graphKit.cpp:1567)
V  [libjvm.so+0x5d69e0]  BarrierSetC2::load_at_resolved(C2Access&, Type const*) const+0x140
V  [libjvm.so+0x5d9870]  BarrierSetC2::load_at(C2Access&, Type const*) const+0xd0
V  [libjvm.so+0xc66db0]  GraphKit::access_load_at(Node*, Node*, TypePtr const*, Type const*, BasicType, unsigned long)+0xb0
V  [libjvm.so+0x10b1304]  LibraryCallKit::inline_unsafe_access(bool, BasicType, LibraryCallKit::AccessKind, bool)+0xb40
V  [libjvm.so+0x10d828c]  LibraryIntrinsic::generate(JVMState*)+0x18c
V  [libjvm.so+0xa5bf50]  Parse::do_call()+0x280
V  [libjvm.so+0x138404c]  Parse::do_one_bytecode()+0x37c
V  [libjvm.so+0x1372dec]  Parse::do_one_block()+0x1ec
V  [libjvm.so+0x137427c]  Parse::do_all_blocks()+0x13c
V  [libjvm.so+0x137799c]  Parse::Parse(JVMState*, ciMethod*, float)+0x9dc
V  [libjvm.so+0x7460d4]  ParseGenerator::generate(JVMState*)+0x174
V  [libjvm.so+0xa5bf50]  Parse::do_call()+0x280
V  [libjvm.so+0x138404c]  Parse::do_one_bytecode()+0x37c
V  [libjvm.so+0x1372dec]  Parse::do_one_block()+0x1ec
V  [libjvm.so+0x137427c]  Parse::do_all_blocks()+0x13c
V  [libjvm.so+0x137799c]  Parse::Parse(JVMState*, ciMethod*, float)+0x9dc
V  [libjvm.so+0x7460d4]  ParseGenerator::generate(JVMState*)+0x174
V  [libjvm.so+0xa5bf50]  Parse::do_call()+0x280
V  [libjvm.so+0x138404c]  Parse::do_one_bytecode()+0x37c
V  [libjvm.so+0x1372dec]  Parse::do_one_block()+0x1ec
V  [libjvm.so+0x137427c]  Parse::do_all_blocks()+0x13c
V  [libjvm.so+0x137799c]  Parse::Parse(JVMState*, ciMethod*, float)+0x9dc
V  [libjvm.so+0x7460d4]  ParseGenerator::generate(JVMState*)+0x174
V  [libjvm.so+0xa5bf50]  Parse::do_call()+0x280
V  [libjvm.so+0x138404c]  Parse::do_one_bytecode()+0x37c
V  [libjvm.so+0x1372dec]  Parse::do_one_block()+0x1ec
V  [libjvm.so+0x137427c]  Parse::do_all_blocks()+0x13c
V  [libjvm.so+0x137799c]  Parse::Parse(JVMState*, ciMethod*, float)+0x9dc
V  [libjvm.so+0x7460d4]  ParseGenerator::generate(JVMState*)+0x174
V  [libjvm.so+0x74b6c0]  CallGenerator::do_late_inline_helper()+0x8d0
V  [libjvm.so+0x8e308c]  Compile::inline_incrementally_one()+0xd8
V  [libjvm.so+0x8e3d40]  Compile::inline_incrementally(PhaseIterGVN&)+0x260
V  [libjvm.so+0x8e5aac]  Compile::Optimize()+0x2bc
V  [libjvm.so+0x8e8e4c]  Compile::Compile(ciEnv*, ciMethod*, int, Options, DirectiveSet*)+0x14c8
V  [libjvm.so+0x743e30]  C2Compiler::compile_method(ciEnv*, ciMethod*, int, bool, DirectiveSet*)+0x17c
V  [libjvm.so+0x8f4b8c]  CompileBroker::invoke_compiler_on_method(CompileTask*)+0x7dc
V  [libjvm.so+0x8f5764]  CompileBroker::compiler_thread_loop()+0x584
V  [libjvm.so+0xd7486c]  JavaThread::thread_main_inner()+0xcc
V  [libjvm.so+0x15b62b0]  Thread::call_run()+0xac
V  [libjvm.so+0x13275ac]  thread_native_entry(Thread*)+0x12c
C  [libpthread.so.0+0x7928]  start_thread+0x188
Comments
Changeset: 0c1b254b Author: Daniel Lundén <dlunden@openjdk.org> Committer: Tobias Hartmann <thartmann@openjdk.org> Date: 2024-03-25 12:04:44 +0000 URL: https://git.openjdk.org/jdk/commit/0c1b254be9ddd3883313f80b61229eacf09aa862
25-03-2024
I spent a few days trying to create a standalone reproducer, but it proved difficult to reproduce outside of the context in which the issue originally appeared. If anyone has any ideas and want to take a look, please feel free to do so (I summarized what I've found so far in the issue description). It is probably not critical to find a reproducer given that the `assert` is incorrect, and not the compilation itself.
21-03-2024
A pull request was submitted for review. URL: https://git.openjdk.org/jdk/pull/18434 Date: 2024-03-21 15:31:58 +0000
21-03-2024
Yes, I agree that we must remove the assert.
27-02-2024
My suggestion will be to remove the assert so that all types of DecodeN node's input will be recorded for later IGVN (as current product VM does). I thought about adding `assert(ld->in(1)->type()->is_narrowoop()` but DecodeNNode::Value() already has this assert.
27-02-2024
Thanks [~kvn]. It is a bit surprising to me that tier1 through tier5 testing does not trigger the assert. I will find a reproducer and create a regression test before creating a pull request. Thanks again!
27-02-2024
> In all cases the input node is a Phi: 19327 Phi === 13596 13593 13538 [[ 19328 17528 17444 14217 19909 ]] #narrowoop: java/lang/String (java/io/Serializable,java/lang/Comparable,java/lang/CharSequence,java/lang/constant/Constable,java/lang/constant/ConstantDesc):exact * !orig=[13599] !jvms: NativeImageBFDNameProvider$BFDMangler::mangleType @ bci:44 (line 748) The value which StoreN node stores can be any node: ConN, LoadN, Phi. What is important is that it is compressed as in this case: "#narrowoop: java/lang/String". So the code is correct (and assert is too strong). LoadNode::Identity() calls `can_see_stored_value()` which can give back narrow value from StoreN node. Which works as intended. I was wondering why we did not hit this issue before but, as Daniel pointed, the assert was added only recently: JDK-8310524.
27-02-2024
Thanks [~kvn] and [~dnsimon]. I believe the assert is too strong (I added it not too long ago), but I'll have a look and try to create a reproducer.
26-02-2024
Thank, Doug. It could be loads from `Pair` class `names` object when we build new `HosteMethod:`names.getLeft(), names.getRight()`. So we need reproducer which inline a method which do such loads but somehow `LoadNode::Ideal()` will find values (Store nodes into may be local not escaped object) to trigger the assert.
24-02-2024
The source for HostedMethod::lambda$create0$0 is here: https://github.com/oracle/graal/blob/d6243acce05af06b17d63dddb2972e739141dc6d/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/meta/HostedMethod.java#L146-L164
23-02-2024
Or we can add `can_reshape` check in `LoadNode::Ideal()` to avoid such LoadN node transformations during Parse.
23-02-2024
During Parse the only way you get DecodeN from LoadNode::make() if it see LoadN node there: https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/memnode.cpp#L972 The assert verifies that. I see at line `memnode.cpp#L972` is that the LoadN check is done before we call `transform(load)` which may convert this node to value from previous Store node (or array copy) or TOP. I thought that we guard such optimizations with `can_reshape` to do it only in IGVN. But looking on `LoadNode::Ideal()` I don't see such check for `find_previous_store()` call. Technically the value from StoreN will be compressed and executing DecodeN on it is correct so we DO need to generate DecodeN and checking LoadN before `transform()` is correct. In this sense the assert indeed is strong. But before jumping to conclusion I think we should add additional dumping in this part of code to see this part of graph. My main concern is that could be some kind of memory corruption because we never hit this assert before. [~dnsimon] or [~never] Is it possible to make some sense about Java code shape for com.oracle.svm.hosted.meta.HostedMethod::lambda$create0$0?
23-02-2024
FWIW it looks like the assert in question was introduced in JDK-8310524 (cc: [~dlunden]).
21-02-2024