RFR: 8263164: assert(_base >= VectorA && _base <= VectorZ) failed: Not a Vector while calling StoreVectorNode::memory_size()
Jie Fu
jiefu at openjdk.java.net
Wed Mar 10 04:21:05 UTC 2021
On Wed, 10 Mar 2021 00:41:44 GMT, Vladimir Kozlov <kvn at openjdk.org> wrote:
>>> I don't think it is correct place to fix it. Why checks in StoreNode(and LoadNode)::Value() does not work for vectors?:
>>> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/memnode.cpp#L2702
>>>
>>> Did we forgot to put this nodes on IGVN worklist somewhere?
>>
>> Hi @vnkozlov ,
>>
>> Thanks for your review.
>>
>> After more investigation, I found the same issue for non-vector store nodes.
>> For example, StoreN was also observed with in(MemNode::ValueIn)->bottom_type() == Type::TOP during PhaseIterGVN::optimize().
>>
>> The nodes should have been pushed on the IGVN worklist since it happened when StoreN was popped from the IGVN worklist.
>> So I think StoreVectorNode::memory_size() should also work when in(MemNode::ValueIn)->bottom_type() == Type::TOP during PhaseIterGVN::optimize().
>>
>> By the way, I'm not clear why you say the checks in StoreNode(and LoadNode)::Value() does not work for vectors?
>> Could you make it more clearer?
>> Thanks.
>>
>> Best regards,
>> Jie
>
> What I mean is IGVN should replace nodes which have TOP input with TOP and remove them from graph as dead nodes.
> `Value()` methods do replacement after checking node's inputs for TOP. And `Ideal()` method should check for TOP and immediately return to avoid unneeded transformations (or guard transformation code with `!= TOP` check) and let `Value()` do replacement with TOP.
>
> In case of memory nodes `MemNode::Ideal_common()` do such checks (but only for ctrl, mem and adr inputs):
> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/memnode.cpp#L286
>
> To my surprise `StoreNode::Ideal()` does not check `in(MemNode::ValueIn)` input for TOP at all. May be because in normal case we don't do much for `in(MemNode::ValueIn)` when processing Store nodes and eventually `StoreNode::Value()` will replace node with TOP.
>
> Yes, your fix works for `Vector` nodes. But as you saw normal `Store` nodes also can have TOP value input and we do useless transformations until StoreNode::Value() calls. I think the better fix is to check ` in(MemNode::ValueIn)` for TOP in `StoreNode::Ideal()`.
>
> And we need to test it hard because it is very old code.
Hi @vnkozlov ,
Unfortunately, it doesn't work to avoid store->in(MemNode::ValueIn)->bottom_type() == Type::TOP by just improving StoreNode::Ideal().
TOP value seems hard to be avoided for store nodes during C2's optimization phase.
So I think we should support StoreVectorNode::memory_size() for that case, which has already been supported by non-vector stores.
And I suggest to keep StoreNode::Ideal() as it is to lower the risk.
What do you think?
Thanks.
Best regards,
Jie
--------------------------------------
To show you how this can happen, please run vmTestbase/nsk/jdi/StepEvent/_itself_/stepEvent004/stepEvent004.java several times with the following change:
diff --git a/src/hotspot/share/opto/memnode.cpp b/src/hotspot/share/opto/memnode.cpp
index 440e29f..28debe1 100644
--- a/src/hotspot/share/opto/memnode.cpp
+++ b/src/hotspot/share/opto/memnode.cpp
@@ -2617,6 +2617,9 @@ Node *StoreNode::Ideal(PhaseGVN *phase, bool can_reshape) {
Node* mem = in(MemNode::Memory);
Node* address = in(MemNode::Address);
Node* value = in(MemNode::ValueIn);
+
+ if (value && value->is_top()) return NULL;
+
// Back-to-back stores to same address? Fold em up. Generally
// unsafe if I have intervening uses... Also disallowed for StoreCM
// since they must follow each StoreP operation. Redundant StoreCMs
diff --git a/src/hotspot/share/opto/memnode.hpp b/src/hotspot/share/opto/memnode.hpp
index b9ee30f..ea878f9 100644
--- a/src/hotspot/share/opto/memnode.hpp
+++ b/src/hotspot/share/opto/memnode.hpp
@@ -603,6 +603,19 @@ public:
#endif
}
+ virtual int memory_size() const {
+ if (in(MemNode::ValueIn)->bottom_type() == Type::TOP) {
+ tty->print_cr("jiefu catch it ...");
+ this->dump();
+ assert(false, "Find top in StoreNode");
+ }
+#ifdef ASSERT
+ return type2aelembytes(memory_type(), true);
+#else
+ return type2aelembytes(memory_type());
+#endif
+ }
+
// Polymorphic factory method
//
// We must ensure that stores of object references will be visible
In my experiment, I got the failure like this
jiefu catch it ...
21550 StoreI === 15446 15451 21551 1 [[ 15454 ]]
#
# A fatal error has been detected by the Java Runtime Environment:
#
# Internal Error (/home/jvm/jdk/src/hotspot/share/opto/memnode.hpp:610), pid=20324, tid=20395
# assert(false) failed: Find top in StoreNode
#
Current thread (0x00007f8900308b40): JavaThread "C2 CompilerThread1" daemon [_thread_in_native, id=20395, stack(0x00007f891a5ac000,0x00007f891a6ad000)]
Current CompileTask:
C2: 2238 1321 % 4 nsk.share.jdi.sde.SDEDebugger::compareLocations @ 66 (381 bytes)
Stack: [0x00007f891a5ac000,0x00007f891a6ad000], sp=0x00007f891a6a8280, free space=1008k
Native frames: (J=compiled Java code, A=aot compiled Java code, j=interpreted, Vv=VM code, C=native code)
V [libjvm.so+0x91a452] StoreNode::memory_size() const+0xe2
V [libjvm.so+0x1282223] InitializeNode::stores_are_sane(PhaseTransform*) [clone .part.212]+0x123
V [libjvm.so+0x128fef8] InitializeNode::capture_store(StoreNode*, long, PhaseGVN*, bool)+0x268
V [libjvm.so+0x12988a3] StoreNode::Ideal(PhaseGVN*, bool)+0x533
V [libjvm.so+0x144564a] PhaseIterGVN::transform_old(Node*)+0xba
V [libjvm.so+0x143f72d] PhaseIterGVN::optimize()+0x7d
V [libjvm.so+0x979834] Compile::Optimize()+0x204
V [libjvm.so+0x97c134] Compile::Compile(ciEnv*, ciMethod*, int, bool, bool, bool, bool, DirectiveSet*)+0x1864
V [libjvm.so+0x7e253a] C2Compiler::compile_method(ciEnv*, ciMethod*, int, bool, DirectiveSet*)+0x10a
V [libjvm.so+0x98a4c6] CompileBroker::invoke_compiler_on_method(CompileTask*)+0xbf6
V [libjvm.so+0x98b058] CompileBroker::compiler_thread_loop()+0x4b8
V [libjvm.so+0x17d120a] JavaThread::thread_main_inner()+0x2fa
V [libjvm.so+0x17d1527] JavaThread::run()+0x2b7
V [libjvm.so+0x17d5fe8] Thread::call_run()+0xf8
V [libjvm.so+0x13c5866] thread_native_entry(Thread*)+0x116
-------------
PR: https://git.openjdk.java.net/jdk/pull/2867
More information about the hotspot-compiler-dev
mailing list