RFR: 8340602: C2: LoadNode::split_through_phi might exhaust nodes in case of base_is_phi [v2]

[Daohan Qu]

On Tue, 24 Sep 2024 05:23:13 GMT, Daohan Qu <dqu at openjdk.org> wrote:

>> # Description 
>> 
>> [JDK-6934604](https://github.com/openjdk/jdk/commit/b4977e887a53c898b96a7d37a3bf94742c7cc194) introduces the flag `AggressiveUnboxing` in jdk8u, and [JDK-8217919](https://github.com/openjdk/jdk/commit/71759e3177fcd6926bb36a30a26f9f3976f2aae8) enables it by default in jdk13u.
>> 
>> But it seems that JDK-6934604 forgets to check duplicate `PhiNode` generated in the function `LoadNode::split_through_phi(PhaseGVN *phase)` (in `memnode.cpp`) in the case that `base` is phi but `mem` is not phi. More exactly, `LoadNode::Identity(PhaseTransform *phase)` doesn't search for `PhiNode` in the correct region in that case.
>> 
>> This might cause infinite split in the case of a loop, which is similar to the bugs fixed in [JDK-6673473](https://github.com/openjdk/jdk/commit/30dc0edfc877000c0ae20384f228b45ba82807b7). The infinite split results in "Out of nodes" and make the method "not compilable".
>> 
>> Since JDK-8217919 (in jdk13u), all the later versions of jdks are affected by this bug when the expected optimization pattern appears in the code. For example, the following three micro-benchmarks running with
>> 
>> 
>> make test \
>>   TEST="micro:org.openjdk.bench.java.util.stream.tasks.IntegerMax.Bulk.bulk_seq_inner micro:org.openjdk.bench.java.util.stream.tasks.IntegerMax.Lambda.bulk_seq_lambda micro:org.openjdk.bench.java.util.stream.tasks.IntegerMax.Lambda.bulk_seq_methodRef" \
>>   MICRO="FORK=1;WARMUP_ITER=2" \
>>   TEST_OPTS="VM_OPTIONS=-XX:+UseParallelGC"
>> 
>> 
>>  shows performance improvement after this PR applied. (`-XX:+UseParallelGC` is only for reproduce this bug, all the bms in the following table are run with this option.)
>> 
>> |benchmark (throughput, unit: ops/s)| jdk-before-this-patch | jdk-after-this-patch |
>> |---|---|---|
>> |org.openjdk.bench.java.util.stream.tasks.IntegerMax.Bulk.bulk_seq_inner | 26.452 ±(99.9%) 0.185 ops/s | 62.060 ±(99.9%) 0.878 ops/s |
>> |org.openjdk.bench.java.util.stream.tasks.IntegerMax.Lambda.bulk_seq_lambda | 26.922 ±(99.9%) 1.710 ops/s | 67.961 ±(99.9%) 0.850 ops/s |
>> |org.openjdk.bench.java.util.stream.tasks.IntegerMax.Lambda.bulk_seq_methodRef | 28.382 ±(99.9%) 1.021 ops/s | 67.998 ±(99.9%) 0.751 ops/s |
>> 
>> # Reproduction
>> 
>> Compiled and run the reduced test case `Test.java` in the appendix below using 
>> 
>> 
>> java -Xbatch -XX:+UnlockDiagnosticVMOptions -XX:+LogCompilation -XX:LogFile=comp.log -XX:+UseParallelGC Test
>> 
>> 
>> and you could find that `Test$Obj.calc` is tagged with `make_not_compilable` and...
>
> Daohan Qu has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision:
> 
>   Check duplicate split in case of base_is_phi

Hi @vnkozlov , I noticed that you have fixed a similar bug in [JDK-6673473](https://github.com/openjdk/jdk/commit/30dc0edfc877000c0ae20384f228b45ba82807b7). Could you please review this PR? Thanks a lot!

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

PR Comment: https://git.openjdk.org/jdk/pull/21134#issuecomment-2370355269