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

[Jie Fu]

On Thu, 11 Mar 2021 03:50:09 GMT, Vladimir Kozlov <kvn at openjdk.org> wrote:

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

Hi @vnkozlov , @XiaohongGong and @theRealELiu ,

If I read the code correctly, StoreNode::Idea can't fix this kind of bugs.

This is because we may see other store nodes with value=top on the IGVN list [1]

StoreNode::Ideal
  |
  `InitializeNode::capture_store
     |
     `InitializeNode::stores_are_sane
        |
        `st->as_Store()->memory_size()  <-- st's value may be TOP (e.g., when it is on the IGVN list waiting bo be transformed)

Fixing in StoreNode::Idea can just bypass the top value for the current store node during IGVN optimization.
But it may still fail when other nodes with top value queuing on the IGVN list.

So we should support vector's memory_size()/element_size()/length()/length_in_bytes() for that case.

vect_type() has been improved based on @XiaohongGong 's comments.
And since StoreNode::Idea won't fix the problem, I suggest to keep StoreNode::Ideal() as it is to lower the risk.
What do you think?

Testing:
  - tier1~tier3 on Linux/x64, no regression
  - nightly testing of jdk/incubator/vector

Thanks.
Best regards,
Jie


[1] https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/memnode.cpp#L4467

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

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