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

please review the following patch:
https://bugs.openjdk.java.net/browse/JDK-8233164
http://cr.openjdk.java.net/~thartmann/8233164/webrev.00/

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