jdk

jdk copied to clipboard

Hi can I have a review for this fix? LoadBNode::Ideal crashes after performing GVN right after EA. The bad IR is as follows:

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:

Thanks for your patience.

---

Progress

• [ ] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• [x] Change must not contain extraneous whitespace
• [x] Commit message must refer to an issue

Issue

• JDK-8288204: GVN Crash: assert() failed: correct memory chain

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9777/head:pull/9777
$ git checkout pull/9777

Update a local copy of the PR:
$ git checkout pull/9777
$ git pull https://git.openjdk.org/jdk pull/9777/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9777

View PR using the GUI difftool:
$ git pr show -t 9777

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9777.diff

:wave: Welcome back yyang! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@kelthuzadx The following label will be automatically applied to this pull request:

• hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

Webrevs

• 04: Full - Incremental (04c082b8)
• 03: Full (9bd483d7)
• 02: Full - Incremental (2d9c3c56)
• 01: Full - Incremental (063d2468)
• 00: Full (cecb86f8)

With your fix, correctness still depends on the order in which nodes are processed by IGVN, right? Wouldn't this still reproduce with -XX:+StressIGVN?

1 LoadB === 1115 1046 969 [[ 972 ]] @b

Hi @TobiHartmann , this patch works well with StressIGVN. There is an explicit dependency path

https://github.com/openjdk/jdk/blob/b17a745d7f55941f02b0bdde83866aa5d32cce07/src/hotspot/share/opto/memnode.cpp#L322-L327

i.e. load node delayed its idealization until its memory input is processed. This means, MergeMem#1046 and its related node were always processed before processing load node. That's why we saw load->in(Addr) was changed from 969 to 473.

 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)
 

I just checked and I can still reproduce the issue with your fix. Simply run: java -Xbatch -XX:CompileCommand=compileonly,Test::test -XX:+StressIGVN -XX:RepeatCompilation=10000 Test

So the root cause must be something else.

@kelthuzadx This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Comment to keep this open. @kelthuzadx, let me know if you need help with reproducing this.

Comment to keep this open. @kelthuzadx, let me know if you need help with reproducing this.

Yes, I'm able to reproduce this, I'm working on other stuff, I will be back at the end of this month

With your fix, correctness still depends on the order in which nodes are processed by IGVN, right? Wouldn't this still reproduce with -XX:+StressIGVN?

You are right, in the current fix, the correctness of LoadB#971(Ctrl Mem Addr) depends on idealization orders, I need more investigating.

I saw a similar crash scenario at https://bugs.openjdk.org/browse/JDK-8233164

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

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

In this case, we are not able to find precise _src_type(after fix) from MergeMem. The full story begins at the creation of LoadBNode in array_copy_forward:

 971  LoadB  ===  958  242  969

We can not find alias index of 969 from current 338#ArrayCopy->in(Mem), so generated LoadB has base memory, i.e. 242#Proj of 337.

Later, ArrayCopyNode::finish_transform replaces 242#Proj with 337#ArrayCopy->in(Mem)(i.e. 1046, originated from 466) when idealizing 337#ArrayCopy. Now we see the following LoadB at the first IGVN iteration:

971  LoadB  ===  1115  1046  969 

The rest of the story is as mentioned in PR content: we lookup an alias index of 969 from 1046#MergeMem and got 1109 whose type is byte[int:>=0]:exact+any *, and 969 is replaced with 473 whose type is byte[int:8]:NotNull:exact+any *,iid=177.

971  LoadB  ===  1115  1046  969 
 971  LoadB  ===  1115  1109  969 
 971  LoadB  ===  1115  1109  969 
 971  LoadB  ===  1115  1109  473  type mismatch, crash at optimize_memory_chain
...

In short, the problem is, once ArrayCopyNode is expanded(idealized), we are not able to know precise _src_type/_dest_type from LoadNode.

I'm not an expert on this, my two thoughts at the moment are

1. Make MemNode::_adr_type product so that we know what kind of memory is being addressed from Load node or
2. In optimize_memory_chain, we always clone the Phi with our address type if we found type mismatch between memory of Load and address type of load.

@TobiHartmann Do you have any ideas or comments? Thanks.

⚠️ @kelthuzadx This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

EA may incorrectly processing LoadB#971 when it splits unique memory slice for allocation instance. LoadB#971 memory should not reference Phi with different type and EA should should look through MergeMem nodes when creating unique memory slice. If LoadB#971 is load from different object (or merge of objects). Then It's AddP node should not be change to instance specific one. Would be nice to see this subgraph just after EA.

EA may incorrectly processing LoadB#971 when it splits unique memory slice for allocation instance. LoadB#971 memory should not reference Phi with different type and EA should should look through MergeMem nodes when creating unique memory slice. If LoadB#971 is load from different object (or merge of objects). Then It's AddP node should not be change to instance specific one. Would be nice to see this subgraph just after EA.

@vnkozlov LoadB#971 is not processed by EA, it was expanded by ArrayCopyNode#338(which is processed by EA) in arraycopy_forward:

MergeMem#466
   |
   |
   v
ArrayCopy#337
          |
          v
       Proj#242
          |
          |
          v
       ArrayCopy#338

https://github.com/openjdk/jdk/blob/78454b69da1434da18193d32813c59126348c9ea/src/hotspot/share/opto/arraycopynode.cpp#L383-L408

mem is Proj#242, mm(960 MergeMem === _ 1 242 1 1 967 [[]]) is based on mem, atp_src and atp_dest are precise types(byte[int:8]:NotNull:exact+any *,iid=177) after JDK-8233164. When generating LoadB#971, we can not find atp_src alias index(32) from mm, so it uses base memory(Proj#242) as its memory input:

242  Proj  ===  337  [[ ... ]] #2  Memory: @BotPTR *+bot, idx=Bot; 
971  LoadB  ===  958  242  969

Then, Proj#242 is replaced by MergeMem#466 when idealizing ArrayCopy#337

971  LoadB  ===  1115  1046  969  <-- MergeMem#1046 is originated from MergeMem#466

At the first idealization of LoadB#971, we lookup the alias type of AddP#969(byte[int:>=0]:NotNull:exact *) from MergeMem#1046, now we found Phi#1109, which has alias type byte[int:>=0]:NotNull:exact *。

ArrayCopy
     |
     |
     v
   CheckCastPP#335 (byte[int:8]:NotNull:exact *,iid=177)
    |
    |
   CastPP#250 (byte[int:>=0]:NotNull:exact *)

 969  AddP  === _  250  250  585 
 971  LoadB  ===  1115  1109  969 

Later, AddP#969 is replaced by 473, it has alias type byte[int:8]:NotNull:exact *,iid=177. At the second idealization of LoadB#971, we replace its address input with AddP#473

969  AddP  === _  335  335  585 
473  AddP  === _  335  335  585
971  LoadB  ===  1115  1109  473

Now Phi#1109 and AddP#473 have mismatched types, we finally hit the assertion.

Thank you for providing additional information. I need to look on this more.

te[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 arra

Make sense. But I guess we should cast more beyond the array parameter, i.e. the offset should be cast as well?

Because we have a Phi#1109(byte[int:>=0]:exact+any*) and AddP#473(byte[int:8]:NotNull:exact[0] *,iid=177) with fixed offset due to ConI#585

 335  CheckCastPP  ===  331  461  [[... ]]  #byte[int:8]:NotNull:exact *,iid=177 
 585  ConL  ===  0  [[ 473  1054  970 ]]  #long:18
 473  AddP  === _  335  335  585  #byte[int:8]:NotNull:exact[0] *,iid=177

@kelthuzadx Please do not rebase or force-push to an active PR as it invalidates existing review comments. All changes will be squashed into a single commit automatically when integrating. See OpenJDK Developers’ Guide for more information.

Thanks for making these changes.

Several tests (for example, compiler/arraycopy/TestArrayCopyAsLoadsStores.java) are now failing with:

# A fatal error has been detected by the Java Runtime Environment:
#
#  Internal Error (workspace/open/src/hotspot/share/opto/phaseX.cpp:843), pid=3983761, tid=3983777
#  assert(i->_idx >= k->_idx) failed: Idealize should return new nodes, use Identity to return old nodes
#
# JRE version: Java(TM) SE Runtime Environment (21.0) (fastdebug build 21-internal-LTS-2022-12-23-0641545.tobias.hartmann.jdk2)
# Java VM: Java HotSpot(TM) 64-Bit Server VM (fastdebug 21-internal-LTS-2022-12-23-0641545.tobias.hartmann.jdk2, mixed mode, sharing, tiered, compressed oops, compressed class ptrs, g1 gc, linux-amd64)
# Problematic frame:
# V  [libjvm.so+0x179ad0c]  PhaseGVN::transform_no_reclaim(Node*)+0xec

Current CompileTask:
C2:   2222  478    b  4       compiler.arraycopy.TestArrayCopyAsLoadsStores::m14 (9 bytes)

Stack: [0x00007f86dc7f5000,0x00007f86dc8f6000],  sp=0x00007f86dc8f20e0,  free space=1012k
Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
V  [libjvm.so+0x179ad0c]  PhaseGVN::transform_no_reclaim(Node*)+0xec  (phaseX.cpp:843)
V  [libjvm.so+0x141be0f]  LibraryCallKit::inline_arraycopy()+0x71f  (library_call.cpp:5289)
V  [libjvm.so+0x1438712]  LibraryIntrinsic::generate(JVMState*)+0x302  (library_call.cpp:115)
V  [libjvm.so+0xcbfbe9]  Parse::do_call()+0x389  (doCall.cpp:662)
V  [libjvm.so+0x176c5f8]  Parse::do_one_bytecode()+0x638  (parse2.cpp:2704)
V  [libjvm.so+0x175a734]  Parse::do_one_block()+0x844  (parse1.cpp:1555)
V  [libjvm.so+0x175b697]  Parse::do_all_blocks()+0x137  (parse1.cpp:707)
V  [libjvm.so+0x176021d]  Parse::Parse(JVMState*, ciMethod*, float)+0xb3d  (parse1.cpp:614)
V  [libjvm.so+0x918c40]  ParseGenerator::generate(JVMState*)+0x110  (callGenerator.cpp:99)
V  [libjvm.so+0xb0275d]  Compile::Compile(ciEnv*, ciMethod*, int, Options, DirectiveSet*)+0x168d  (compile.cpp:760)
V  [libjvm.so+0x916857]  C2Compiler::compile_method(ciEnv*, ciMethod*, int, bool, DirectiveSet*)+0x4e7  (c2compiler.cpp:113)
V  [libjvm.so+0xb0fa2c]  CompileBroker::invoke_compiler_on_method(CompileTask*)+0xa7c  (compileBroker.cpp:2237)
V  [libjvm.so+0xb107e8]  CompileBroker::compiler_thread_loop()+0x5d8  (compileBroker.cpp:1916)
V  [libjvm.so+0x107d066]  JavaThread::thread_main_inner()+0x206  (javaThread.cpp:709)
V  [libjvm.so+0x1a723c0]  Thread::call_run()+0x100  (thread.cpp:224)
V  [libjvm.so+0x1712553]  thread_native_entry(Thread*)+0x103  (os_linux.cpp:739)

Commenting out transformation in array_copy_forward works now, all test under test/hotspot/jtreg/compiler passed except tests that always failed.

PhaseGVN::transform_no_reclaim still crashes when reverting this patch and only adding mm->transform in array_copy_forward, so at least this is not related to this fix.

Though, I don't see why it causes the crash at first glance..

/reviewers 2

@y1yang0 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8288204: GVN Crash: assert() failed: correct memory chain

Reviewed-by: kvn, thartmann

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 196 new commits pushed to the master branch:

• da75de31841e4b50477774e9efc4f554e1f3e4c0: 8299172: RISC-V: [TESTBUG] Fix stack alignment logic in jvmci RISCV64TestAssembler.java
• 19ce23c6459a452c8d3856b9ed96bfa54a8346ae: Merge
• 33042a49d75011958e5030679433e6b2a779d90a: 8299237: add ArraysSupport.newLength test to a test group
• a80c91d0360864e34569b684cf159e2dcdebeaaf: 8299230: Use https: in links
• 9863f59e1db84f55dc9a1670cd73ec4bfc07bcb0: 8299015: Ensure that HttpResponse.BodySubscribers.ofFile writes all bytes
• 5e001d6ff34e2cc954f824117a73dd39f09a81c1: 8299207: [Testbug] Add back test/jdk/java/awt/Graphics2D/DrawPrimitivesTest.java
• a0a09d56ba4fc6133b423ad29d86fc99dd6dc19b: 8298176: remove OpaqueZeroTripGuardPostLoop once main-loop disappears
• fef70d78baec9ce11d50b9a4c1fb26a1b854ccbf: 8299077: [REDO] JDK-4512626 Non-editable JTextArea provides no visual indication of keyboard focus
• 2f4098e1dc9c97e706d70008e473f9c4496cbc8a: 8299168: RISC-V: Fix MachNode size mismatch for MacroAssembler::_verify_oops*
• 2294f225c074516abd2fecf5c64e2e1a2453bc6f: 8286311: remove boilerplate from use of runTests
• ... and 186 more: https://git.openjdk.org/jdk/compare/7f9c6ce3318aedfd85f12f4002dc442b0b468c27...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@TobiHartmann The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

Looks good to me. Tests now pass (still running). Would still be good to know why the additional transform call is an issue.

Yes, that's somewhat strange. I'm looking more into that and will update in this PR later(or file a new issue if it's really a bug).

MergeMemNode::Identity() will return base memory if there are no real merge of memories. The failed test m14 copies 0 elements. In such case we don't generate loads/stores for forward copy. And calling transform on new mm will just return its base memory which is existing node.

I think ArrayCopyNode::Ideal() did not take such case into account.

MergeMemNode::Identity() will return base memory if there are no real merge of memories. The failed test m14 copies 0 elements. In such case we don't generate loads/stores for forward copy. And calling transform on new mm will just return its base memory which is existing node.

I think ArrayCopyNode::Ideal() did not take such case into account.

In general when you return new node in some Ideal() method you don't call transform() on it. You call transform() on new nodes used to construct a new returned node.

/integrate

Going to push as commit 04591595374e84cfbfe38d92bff4409105b28009. Since your change was applied there have been 201 commits pushed to the master branch:

• 19373b2ff0cd795afa262c17dcb3388fd6a5be59: Merge
• 188911c925e4067c7f912c5ddb6f715bad7a3892: 8299241: jdk/jfr/api/consumer/streaming/TestJVMCrash.java generates unnecessary core file
• 26868c1ac471c3b305b1d15e3075de0baa9319d2: 8299255: Unexpected round errors in FreetypeFontScaler
• 5e861e3965ce110889c8a1782ab0260937dee6ee: 8298645: JNI works with accessibleSelection on a wrong thread
• fd746a2fe0e4c1c056065da93e2be2d8bb4e5428: 8298643: JNI call of getAccessibleRowWithIndex and getAccessibleColumnWithIndex on a wrong thread
• da75de31841e4b50477774e9efc4f554e1f3e4c0: 8299172: RISC-V: [TESTBUG] Fix stack alignment logic in jvmci RISCV64TestAssembler.java
• 19ce23c6459a452c8d3856b9ed96bfa54a8346ae: Merge
• 33042a49d75011958e5030679433e6b2a779d90a: 8299237: add ArraysSupport.newLength test to a test group
• a80c91d0360864e34569b684cf159e2dcdebeaaf: 8299230: Use https: in links
• 9863f59e1db84f55dc9a1670cd73ec4bfc07bcb0: 8299015: Ensure that HttpResponse.BodySubscribers.ofFile writes all bytes
• ... and 191 more: https://git.openjdk.org/jdk/compare/7f9c6ce3318aedfd85f12f4002dc442b0b468c27...master

Your commit was automatically rebased without conflicts.

@y1yang0 Pushed as commit 04591595374e84cfbfe38d92bff4409105b28009.

:bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.