[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

[Vladimir Ivanov]

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