RFR: 8263164: assert(_base >= VectorA && _base <= VectorZ) failed: Not a Vector while calling StoreVectorNode::memory_size()

[Vladimir Kozlov]

On Wed, 10 Mar 2021 04:18:16 GMT, Jie Fu <jiefu at openjdk.org> wrote:

>> 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

Would be nice to see where TOP value is coming from in stepEvent004.java test.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2867