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

[Daohan Qu]

On Wed, 25 Sep 2024 10:25:35 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

> Can you explain in more detail how we fail to cut the infinite splitting and how your patch now solves this?
> 
> I have some initial comments.

Thanks for your comments @chhagedorn ! Let me try to explain it in more detail and hope it helps.

## Before this patch
* ### Expected case (when `mem->is_Phi()` holds)

1. When split succeeds, the `in(Memory)->_idx` and some information of the instance is recorded in the `PhiNode`
https://github.com/openjdk/jdk/blob/47fcf5a3b0796ffeb6407be961ceb552ca2a40f8/src/hotspot/share/opto/memnode.cpp#L1726-L1729

2. Next time when a duplicate split is about to happen and the execution reaches here before `PhiNode` is created:
https://github.com/openjdk/jdk/blob/47fcf5a3b0796ffeb6407be961ceb552ca2a40f8/src/hotspot/share/opto/memnode.cpp#L1676-L1678

3. In `LoadNode::Identity()`, it will search for a duplicate split within the region (`mem->in(0)`) using the information mentioned above. Since there has been a `PhiNode` with same information created as described above, `Identity()` will return it. This results in the code snippet in step 2 to return `nullptr`. That is, `LoadNode::split_through_phi()` returns `nullptr` and no duplicate split happens.
https://github.com/openjdk/jdk/blob/47fcf5a3b0796ffeb6407be961ceb552ca2a40f8/src/hotspot/share/opto/memnode.cpp#L1321-L1327

* ### Buggy case (when `base_is_phi` holds)
1. According to the following code, the region where the new `PhiNode` resides in might not be `mem->in(0)` but `base->in(0)`. So the above search in `LoadNode::Identity()` might not find the duplicate `PhiNode`. This won't be a problem if the IR pattern required for split doesn't exist after a finite number of splits. But once `PhiNode`s form a cycle (in case of a loop in the Java code) and the IR pattern might still exist after splits, failure to find the duplicate will cause the split to happen again and again.
https://github.com/openjdk/jdk/blob/47fcf5a3b0796ffeb6407be961ceb552ca2a40f8/src/hotspot/share/opto/memnode.cpp#L1681-L1710

## After this patch
I just extract the code searching for duplicate `PhiNode` into a function `LoadNode::phi_or_self()` and make it to search in the correct region thus finding the duplicate.

> src/hotspot/share/opto/memnode.cpp line 1257:
> 
>> 1255: //----------------------is_instance_field_load_with_local_phi------------------
>> 1256: bool LoadNode::is_instance_field_load_with_local_phi() {
>> 1257:   if( in(Memory)->is_Phi() && in(Address)->is_AddP() ) {
> 
> Was like that before but usually, we should fix the code style when touching old code like spacing
>  ```suggestion
>   if (in(Memory)->is_Phi() && in(Address)->is_AddP()) {
> 
> or `*` placement (should be at type). There are some more places below which you could also fix with this patch.

Thanks for highlighting that practice!

> src/hotspot/share/opto/memnode.cpp line 1303:
> 
>> 1301:   }
>> 1302: 
>> 1303:   if (is_boxed_value_load_with_local_phi(phase)) {
> 
> Does `t_oop->is_ptr_to_boxed_value()` hold here? Should we assert this?

Since there is not too much code after `t_oop->is_ptr_to_boxed_value()` is checked `is_boxed_value_load_with_local_phi()`, I think this assert might be unnecessary.
https://github.com/openjdk/jdk/pull/21134/files#diff-ec810a1df7150822244e55ee309c86d6cbffe108ae9c72b6d258ea5758677c28R1276-R1280

  bool base_is_phi = (base != NULL) && base->is_Phi();
  bool load_boxed_value = t_oop != NULL && t_oop->is_ptr_to_boxed_value()
                          && C->aggressive_unboxing() && (base != NULL) && (base == address->in(AddPNode::Base))
                          && phase->type(base)->higher_equal(TypePtr::NOTNULL);
  return base_is_phi && load_boxed_value;
  ```

> src/hotspot/share/opto/memnode.cpp line 1305:
> 
>> 1303:   if (is_boxed_value_load_with_local_phi(phase)) {
>> 1304:     intptr_t ignore = 0;
>> 1305:     Node * base = AddPNode::Ideal_base_and_offset(in(Address), phase, ignore);
> 
> There was a null check before for `base`. Is this not required anymore?

I just checked it in `is_boxed_value_load_with_local_phi()` as shown below. If `base` is `nullptr`, this code won't be executed.
https://github.com/openjdk/jdk/pull/21134/files#diff-ec810a1df7150822244e55ee309c86d6cbffe108ae9c72b6d258ea5758677c28R1276-R1280

  bool base_is_phi = (base != NULL) && base->is_Phi();
  bool load_boxed_value = t_oop != NULL && t_oop->is_ptr_to_boxed_value()
                          && C->aggressive_unboxing() && (base != NULL) && (base == address->in(AddPNode::Base))
                          && phase->type(base)->higher_equal(TypePtr::NOTNULL);
  return base_is_phi && load_boxed_value;

> test/hotspot/jtreg/compiler/loopopts/TestInfiniteSplitInCaseOfBaseIsPhi.java line 27:
> 
>> 25:  * @test
>> 26:  * @bug 8340602
>> 27:  * @requires vm.compiler2.enabled
> 
> Since you run with Parallel GC, you should add a requires here:
> Suggestion:
> 
>  * @requires vm.compiler2.enabled & & vm.gc.Parallel

Thanks for reminding! I will update it. (Is there a redundant "&" in the suggested change？:P )

> test/hotspot/jtreg/compiler/loopopts/TestInfiniteSplitInCaseOfBaseIsPhi.java line 38:
> 
>> 36: import java.util.Random;
>> 37: 
>> 38: public class TestInfiniteSplitInCaseOfBaseIsPhi {
> 
> You should fix the indentation to 4 spaces for Java code instead of 2.

Indeed. I'll also update it.

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

PR Comment: https://git.openjdk.org/jdk/pull/21134#issuecomment-2378301426
PR Review Comment: https://git.openjdk.org/jdk/pull/21134#discussion_r1777941250
PR Review Comment: https://git.openjdk.org/jdk/pull/21134#discussion_r1777941439
PR Review Comment: https://git.openjdk.org/jdk/pull/21134#discussion_r1777941352
PR Review Comment: https://git.openjdk.org/jdk/pull/21134#discussion_r1777939803
PR Review Comment: https://git.openjdk.org/jdk/pull/21134#discussion_r1777940261