RFR: 8361140: Missing OptimizePtrCompare check in ConnectionGraph::reduce_phi_on_cmp [v2]

[Christian Hagedorn]

On Thu, 10 Jul 2025 02:11:21 GMT, Guanqiang Han <duke at openjdk.org> wrote:

>> When running with `-XX:-OptimizePtrCompare` (which disables pointer comparison optimization), the compiler may hit an assertion failure in debug builds because `optimize_ptr_compare` is still being called. This violates the intended usage of the flag and leads to unexpected crashes.
>> 
>> This patch adds an early return to `reduce_phi_on_cmp` when `OptimizePtrCompare` is false. Since the optimization relies on `optimize_ptr_compare` for static reasoning about comparisons, there's no benefit in proceeding with `reduce_phi_on_cmp` when this flag is disabled.
>
> Guanqiang Han has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
> 
>  - update modification and add regression test
>  - Merge remote-tracking branch 'upstream/master' into 8361140
>  - 8361140: Missing OptimizePtrCompare check in ConnectionGraph::reduce_phi_on_cmp
>    
>    When running with `-XX:-OptimizePtrCompare` (which disables pointer comparison optimization), the compiler may hit an assertion failure in debug builds because `optimize_ptr_compare` is still being called. This violates the intended usage of the flag and leads to unexpected crashes.
>    
>    This patch adds an early return to `reduce_phi_on_cmp` when `OptimizePtrCompare` is false. Since the optimization relies on `optimize_ptr_compare` for static reasoning about comparisons, there's no benefit in proceeding with `reduce_phi_on_cmp` when this support is disabled.

Thanks for the update! I have some follow-up comments.

src/hotspot/share/opto/escape.cpp line 3280:

> 3278:   const TypeInt* EQ = TypeInt::CC_EQ; // [0] == ZERO
> 3279:   const TypeInt* NE = TypeInt::CC_GT; // [1] == ONE
> 3280:   const TypeInt* UNKNOWN = TypeInt::CC;    // [-1, 0,1]

I suggest to move the `UNKNOWN` definition up and then use `UNKNOWN` as return value which also serves as documentation.

test/hotspot/jtreg/compiler/c2/TestReducePhiOnCmpWithNoOptPtrCompare.java line 29:

> 27:  * @summary Test ConnectionGraph::reduce_phi_on_cmp when OptimizePtrCompare is disabled
> 28:  * @library /test/lib /
> 29:  * @requires vm.debug == true

`OptimizePtrCompare` is a product flag. Thus, you do not need this `requires`.
Suggestion:

test/hotspot/jtreg/compiler/c2/TestReducePhiOnCmpWithNoOptPtrCompare.java line 30:

> 28:  * @library /test/lib /
> 29:  * @requires vm.debug == true
> 30:  * @requires vm.compiler2.enabled

I suggest to also remove this line and additionally pass `-XX:+IgnoreUnrecognizedVMOptions` as flag to the IR framework.
Suggestion:

test/hotspot/jtreg/compiler/c2/TestReducePhiOnCmpWithNoOptPtrCompare.java line 46:

> 44:         TestFramework framework = new TestFramework();
> 45:         Scenario scenario0 = new Scenario(0, "-XX:-OptimizePtrCompare");
> 46:         framework.addScenarios(scenario0).start();

Since you only use one setting, you can directly use `TestFramework.runWithFlags()`. Can you also add `-XX:+VerifyReduceAllocationMerges` for additional verification?

I also suggest to add a copy of scenario 0 at `AllocationMergesTests` and add `-XX:-OptimizePtrCompare` and `-XX:+VerifyReduceAllocationMerges` to the scenario.

test/hotspot/jtreg/compiler/c2/TestReducePhiOnCmpWithNoOptPtrCompare.java line 53:

> 51:         invocations++;
> 52:         Random random = info.getRandom();
> 53:         boolean cond = invocations % 2 == 0;

Why don't you just use `random.nextBoolean()`?

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

PR Review: https://git.openjdk.org/jdk/pull/26125#pullrequestreview-3004309866
PR Review Comment: https://git.openjdk.org/jdk/pull/26125#discussion_r2197647931
PR Review Comment: https://git.openjdk.org/jdk/pull/26125#discussion_r2196840228
PR Review Comment: https://git.openjdk.org/jdk/pull/26125#discussion_r2196840831
PR Review Comment: https://git.openjdk.org/jdk/pull/26125#discussion_r2197643267
PR Review Comment: https://git.openjdk.org/jdk/pull/26125#discussion_r2197645377