RFR: 8288204: GVN Crash: assert() failed: correct memory chain [v3]

Sat Oct 29 00:04:34 UTC 2022

On Mon, 24 Oct 2022 08:13:06 GMT, Yi Yang <yyang at openjdk.org> wrote:

>> Hi can I have a review for this fix? LoadBNode::Ideal crashes after performing GVN right after EA. The bad IR is as follows:
>> 
>> ![image](https://user-images.githubusercontent.com/5010047/183106710-3a518e5e-0b59-4c3c-aba4-8b6fcade3519.png)
>> 
>> The memory input of Load#971 is Phi#1109 and the address input of Load#971 is AddP whose object base is CheckCastPP#335:
>> 
>> The type of Phi#1109 is `byte[int:>=0]:exact+any *` while `byte[int:8]:NotNull:exact+any *,iid=177`  is the type of CheckCastPP#335 due to EA, they have different alias index, that's why we hit the assertion at L226:
>> 
>> https://github.com/openjdk/jdk/blob/b17a745d7f55941f02b0bdde83866aa5d32cce07/src/hotspot/share/opto/memnode.cpp#L207-L226
>> (t is `byte[int:>=0]:exact+any *`, t_adr is  `byte[int:8]:NotNull:exact+any *,iid=177`).
>> 
>> There is a long story. In the beginning, LoadB#971 is generated at array_copy_forward, and GVN transformed it iteratively:
>> 
>>  971  LoadB  ===  1115  1046  969  [[ 972 ]]  @byte[int:8]:NotNull:exact+any *,iid=177, idx=32; #byte !jvms: String::coder @ bci:0 (line 4540) String::getBytes @ bci:1 (line 4453) StringConcatHelper::prepend @ bci:21 (line 354) StringConcatHelper::simpleConcat @ bci:81 (line 425) DirectMethodHandle$Holder::invokeStatic @ bci:11 DelegatingMethodHandle$Holder::reinvoke_L @ bci:14 Invokers$Holder::linkToTargetMethod @ bci:6 Test::test @ bci:121 (line 22)
>>  971  LoadB  ===  1115  1109  969  [[ 972 ]]  @byte[int:8]:NotNull:exact+any *,iid=177, idx=32; #byte !jvms: String::coder @ bci:0 (line 4540) String::getBytes @ bci:1 (line 4453) StringConcatHelper::prepend @ bci:21 (line 354) StringConcatHelper::simpleConcat @ bci:81 (line 425) DirectMethodHandle$Holder::invokeStatic @ bci:11 DelegatingMethodHandle$Holder::reinvoke_L @ bci:14 Invokers$Holder::linkToTargetMethod @ bci:6 Test::test @ bci:121 (line 22)
>>  971  LoadB  ===  1115  1109  969  [[ 972 ]]  @byte[int:8]:NotNull:exact+any *,iid=177, idx=32; #byte !jvms: String::coder @ bci:0 (line 4540) String::getBytes @ bci:1 (line 4453) StringConcatHelper::prepend @ bci:21 (line 354) StringConcatHelper::simpleConcat @ bci:81 (line 425) DirectMethodHandle$Holder::invokeStatic @ bci:11 DelegatingMethodHandle$Holder::reinvoke_L @ bci:14 Invokers$Holder::linkToTargetMethod @ bci:6 Test::test @ bci:121 (line 22)
>>  971  LoadB  ===  1115  1109  473  [[ 972 ]]  @byte[int:8]:NotNull:exact+any *,iid=177, idx=32; #byte !jvms: String::coder @ bci:0 (line 4540) String::getBytes @ bci:1 (line 4453) StringConcatHelper::prepend @ bci:21 (line 354) StringConcatHelper::simpleConcat @ bci:81 (line 425) DirectMethodHandle$Holder::invokeStatic @ bci:11 DelegatingMethodHandle$Holder::reinvoke_L @ bci:14 Invokers$Holder::linkToTargetMethod @ bci:6 Test::test @ bci:121 (line 22)
>> ...
>> 
>> In this case, we get alias index 5 from address input AddP#969, and step it through MergeMem#1046, we found Phi#1109 then, that's why LoadB->in(Mem) is changed from MergeMem#1046 to Phi#1109 (Which finally leads to crash).
>> 
>> 1046  MergeMem  === _  1  160  389  389  1109  1  1  389  1  1  1  1  1  1  1  1  1  1  1  1  1  709  709  709  709  882  888  894  190  190  912  191  [[ 1025  1021  1017  1013  1009  1005  1002  1001  998  996  991  986  981  976  971  966  962  961  960  121  122  123  124  1027 ]]  
>> 
>> 
>> After applying this patch, some related nodes are pushed into the GVN worklist, before stepping through MergeMem#1046, the address input is already changed to AddP#473. i.e., we get alias index 32 from address input AddP#473, and step it through MergeMem#1046, we found StoreB#191 then,LoadB->in(Mem) is changed from MergeMem#1046 to StoreB#191. 
>> 
>>  971  LoadB  ===  1115  1046  969  [[ 972 ]]  @byte[int:8]:NotNull:exact+any *,iid=177, idx=32; #byte !jvms: String::coder @ bci:0 (line 4540) String::getBytes @ bci:1 (line 4453) StringConcatHelper::prepend @ bci:21 (line 354) StringConcatHelper::simpleConcat @ bci:81 (line 425) DirectMethodHandle$Holder::invokeStatic @ bci:11 DelegatingMethodHandle$Holder::reinvoke_L @ bci:14 Invokers$Holder::linkToTargetMethod @ bci:6 Test::test @ bci:121 (line 22)
>>  971  LoadB  ===  1115  1046  969  [[ 972 ]]  @byte[int:8]:NotNull:exact+any *,iid=177, idx=32; #byte !jvms: String::coder @ bci:0 (line 4540) String::getBytes @ bci:1 (line 4453) StringConcatHelper::prepend @ bci:21 (line 354) StringConcatHelper::simpleConcat @ bci:81 (line 425) DirectMethodHandle$Holder::invokeStatic @ bci:11 DelegatingMethodHandle$Holder::reinvoke_L @ bci:14 Invokers$Holder::linkToTargetMethod @ bci:6 Test::test @ bci:121 (line 22)
>>  971  LoadB  ===  1115  1046  473  [[ 972 ]]  @byte[int:8]:NotNull:exact+any *,iid=177, idx=32; #byte !jvms: String::coder @ bci:0 (line 4540) String::getBytes @ bci:1 (line 4453) StringConcatHelper::prepend @ bci:21 (line 354) StringConcatHelper::simpleConcat @ bci:81 (line 425) DirectMethodHandle$Holder::invokeStatic @ bci:11 DelegatingMethodHandle$Holder::reinvoke_L @ bci:14 Invokers$Holder::linkToTargetMethod @ bci:6 Test::test @ bci:121 (line 22)
>>  971  LoadB  ===  1115  191  473  [[ 972 ]]  @byte[int:8]:NotNull:exact+any *,iid=177, idx=32; #byte !jvms: String::coder @ bci:0 (line 4540) String::getBytes @ bci:1 (line 4453) StringConcatHelper::prepend @ bci:21 (line 354) StringConcatHelper::simpleConcat @ bci:81 (line 425) DirectMethodHandle$Holder::invokeStatic @ bci:11 DelegatingMethodHandle$Holder::reinvoke_L @ bci:14 Invokers$Holder::linkToTargetMethod @ bci:6 Test::test @ bci:121 (line 22)
>>  971  LoadB  ===  1115  191  473  [[ 972 ]]  @byte[int:8]:NotNull:exact+any *,iid=177, idx=32; #byte !jvms: String::coder @ bci:0 (line 4540) String::getBytes @ bci:1 (line 4453) StringConcatHelper::prepend @ bci:21 (line 354) StringConcatHelper::simpleConcat @ bci:81 (line 425) DirectMethodHandle$Holder::invokeStatic @ bci:11 DelegatingMethodHandle$Holder::reinvoke_L @ bci:14 Invokers$Holder::linkToTargetMethod @ bci:6 Test::test @ bci:121 (line 22)
>>  971  LoadB  ===  1115  191  473  [[ 972 ]]  @byte[int:8]:NotNull:exact+any *,iid=177, idx=32; #byte !jvms: String::coder @ bci:0 (line 4540) String::getBytes @ bci:1 (line 4453) StringConcatHelper::prepend @ bci:21 (line 354) StringConcatHelper::simpleConcat @ bci:81 (line 425) DirectMethodHandle$Holder::invokeStatic @ bci:11 DelegatingMethodHandle$Holder::reinvoke_L @ bci:14 Invokers$Holder::linkToTargetMethod @ bci:6 Test::test @ bci:121 (line 22)
>>  971  LoadB  ===  1115  191  473  [[ 972 ]]  @byte[int:8]:NotNull:exact+any *,iid=177, idx=32; #byte !jvms: String::coder @ bci:0 (line 4540) String::getBytes @ bci:1 (line 4453) StringConcatHelper::prepend @ bci:21 (line 354) StringConcatHelper::simpleConcat @ bci:81 (line 425) DirectMethodHandle$Holder::invokeStatic @ bci:11 DelegatingMethodHandle$Holder::reinvoke_L @ bci:14 Invokers$Holder::linkToTargetMethod @ bci:6 Test::test @ bci:121 (line 22)
>>  971  LoadB  ===  1115  191  473  [[ 972 ]]  @byte[int:8]:NotNull:exact+any *,iid=177, idx=32; #byte !jvms: String::coder @ bci:0 (line 4540) String::getBytes @ bci:1 (line 4453) StringConcatHelper::prepend @ bci:21 (line 354) StringConcatHelper::simpleConcat @ bci:81 (line 425) DirectMethodHandle$Holder::invokeStatic @ bci:11 DelegatingMethodHandle$Holder::reinvoke_L @ bci:14 Invokers$Holder::linkToTargetMethod @ bci:6 Test::test @ bci:121 (line 22)
>>  971  LoadB  ===  468  191  473  [[ 972 ]]  @byte[int:8]:NotNull:exact+any *,iid=177, idx=32; #byte !jvms: String::coder @ bci:0 (line 4540) String::getBytes @ bci:1 (line 4453) StringConcatHelper::prepend @ bci:21 (line 354) StringConcatHelper::simpleConcat @ bci:81 (line 425) DirectMethodHandle$Holder::invokeStatic @ bci:11 DelegatingMethodHandle$Holder::reinvoke_L @ bci:14 Invokers$Holder::linkToTargetMethod @ bci:6 Test::test @ bci:121 (line 22)
>>  971  LoadB  ===  468  191  473  [[ 972 ]]  @byte[int:8]:NotNull:exact+any *,iid=177, idx=32; #byte !jvms: String::coder @ bci:0 (line 4540) String::getBytes @ bci:1 (line 4453) StringConcatHelper::prepend @ bci:21 (line 354) StringConcatHelper::simpleConcat @ bci:81 (line 425) DirectMethodHandle$Holder::invokeStatic @ bci:11 DelegatingMethodHandle$Holder::reinvoke_L @ bci:14 Invokers$Holder::linkToTargetMethod @ bci:6 Test::test @ bci:121 (line 22)
>>  971  LoadB  ===  390  191  473  [[ 972 ]]  @byte[int:8]:NotNull:exact+any *,iid=177, idx=32; #byte !jvms: String::coder @ bci:0 (line 4540) String::getBytes @ bci:1 (line 4453) StringConcatHelper::prepend @ bci:21 (line 354) StringConcatHelper::simpleConcat @ bci:81 (line 425) DirectMethodHandle$Holder::invokeStatic @ bci:11 DelegatingMethodHandle$Holder::reinvoke_L @ bci:14 Invokers$Holder::linkToTargetMethod @ bci:6 Test::test @ bci:121 (line 22)
>> ...
>> 
>> The well-formed IR looks like this:
>> ![image](https://user-images.githubusercontent.com/5010047/183239456-7096ea66-6fca-4c84-8f46-8c42d10b686a.png)
>> 
>> Thanks for your patience.
>
> Yi Yang has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - fix
>  - always clone the Phi with address type

So the issue is that the chain of casts in `MemNode::optimize_memory_chain()` can't convert bottom array type `byte[int:>=0]:exact+any*` to instance array type `byte[int:8]:NotNull:exact+any *,iid=177`. Mostly because it does not adjust array's parameters. This cast chain simply does not work for arrays.

You removed the cast chain. But you loose check for case when types are really not compatible. `PhiNode::split_out_instance()` does not take into account the current Phi's type when it creates new Phi based on Address type: `PhiNode *nphi = slice_memory(at);`

Instead I think you need additional casts specifically for arrays (following code is simplified example) before comparing types:

    if (t_opp->isa_aryptr() && t->isa_aryptr() && (t_opp->elem() == t->elem())) {
      t->is_aryptr()->cast_to_size(t_opp->isa_aryptr()->size())->cast_to_stable(t_oop->is_stable());
    }

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

PR: https://git.openjdk.org/jdk/pull/9777