[14] RFR(S): 8233164: C2 fails with assert(phase->C->get_alias_index(t) == phase->C->get_alias_index(t_adr)) failed: correct memory chain

[Tobias Hartmann]

Hi Vladimir,

thanks for the quick review!

Best regards,
Tobias

On 20.12.19 12:25, Vladimir Ivanov wrote:
> 
>> http://cr.openjdk.java.net/~thartmann/8233164/webrev.00/
> 
> Looks good.
> 
> Best regards,
> Vladimir Ivanov
> 
>> The problem is that the arraycopy ideal transformation does not correctly wire memory inputs on
>> individual loads from a non-escaping src array.
>>
>> I was able to extract a test from the intermittently failing application that depends on indify
>> string concat (test1). From that, I've created another test that does not depend on Strings (test2).
>>
>> Gory details based on test2:
>> Two subsequent arraycopies copy data from two sources into the same destination. The ideal
>> transformation replaces the first arraycopy by a forward and backward copy (because
>> ArrayCopyNode::array_copy_test_overlap determines that the arrays may overlap). A PhiNode is added
>> to select between the memory outputs of the forward and backward stores:
>> http://hg.openjdk.java.net/jdk/jdk/file/2fbc66ef1a1d/src/hotspot/share/opto/arraycopynode.cpp#l617
>>
>>   574    StoreB    ===  559  572  553  573  [[ 568  576 ]]  @byte[int:0..max-2]:NotNull:exact+any ...
>>   567    StoreB    ===  558  562  565  566  [[ 560  576 ]]  @byte[int:0..max-2]:NotNull:exact+any ...
>>   575    Region    ===  575  558  559  [[ 575  576 ]]
>>   576    Phi    ===  575  567  574  [[]]  #memory  Memory: @byte[int:>=0]:exact+any *, idx=6;
>>
>> The second arraycopy can't be optimized (yet) because the src operand and its length are not known
>> but once Escape Analysis is executed, it determines that both source arrays are non-escaping. Now
>> the ideal transformation is able to replace the second arraycopy by loads/stores as well.
>>
>> The memory slice for the first load is selected from the MergeMem based on its address type
>> ('atp_src') which is the general byte[int:>=0] slice due to a CastPP that hides the type of the
>> non-escaping source array. As a result, we wire it to the byte[int:>=0] memory Phi that was created
>> when optimizing the first arraycopy:
>>
>>   305    CheckCastPP ===  302  300  [[ 385  316  420  494 ]]  #byte[int:1]:NotNull:exact *,iid=288
>>   494    CastPP    ===  486  305  [[ 526  514  626  626 ]]  #byte[int:>=0]:NotNull:exact *
>>   626    AddP    === _  494  494  68  [[ 629 ]]
>>   576    Phi    ===  575  567  574  [[ 560  628  629 ]]  #memory  Memory: @byte[int:>=0]:exact+any
>>   629    LoadB    ===  508  576  626  [[]]  @byte[int:>=0]:NotNull:exact+any *, idx=6; #byte
>>
>> That's obviously incorrect because now the LoadB has an address input into a non-escaping source
>> with type byte[int:1], iid=288 and a memory input from the independent byte[int:>=0] slice:
>> http://cr.openjdk.java.net/~thartmann/8233164/graph_Failure.png
>>
>> Basically, 629 LoadB uses memory from 47 AllocateArray when loading from an address into
>> non-escaping 288 AllocateArray. We then hit an assert in MemNode::optimize_memory_chain.
>>
>> The fix is to use _src_type/_dest_type (introduced by JDK-8076188) as address types for the loads
>> and stores. These will have the correct type if the source or destination array is non-escaping.
>>
>> The affected code is in there for a long time but I can only reproduce this back until JDK 13 b08
>> when JDK-8217990 was fixed. I don't think that fix is related but since the crash greatly depends on
>> the order in which nodes are processed by IGVN, it probably just triggers the issue (for example, if
>> we process a CastPP node first, it goes away and we get the correct type from the CheckCastPP). The
>> code was also changed significantly by JDK-8210887 in JDK 12.
>>
>> Thanks,
>> Tobias
>>