RFR: 8320649: C2: Optimize scoped values [v15]

[Roland Westrelin]

On Thu, 18 Apr 2024 09:56:56 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Roland Westrelin has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 22 commits:
>> 
>>  - Merge branch 'master' into JDK-8320649
>>  - review
>>  - test fix
>>  - test fix
>>  - Merge branch 'master' into JDK-8320649
>>  - whitespaces
>>  - review
>>  - Merge branch 'master' into JDK-8320649
>>  - review
>>  - 32 bit build fix
>>  - ... and 12 more: https://git.openjdk.org/jdk/compare/bfff02ee...a4ffc11e
>
> test/hotspot/jtreg/compiler/c2/irTests/TestScopedValue.java line 185:
> 
>> 183:     @IR(counts = {IRNode.IF, "<= 4", IRNode.LOAD_P_OR_N, "<= 5" })
>> 184:     public static void testFastPath5() {
>> 185:         Object unused = svObject.get(); // cannot be removed if result not used
> 
> why? could there be some exception? please add comment why.

Yes `get()` can throw.

> test/hotspot/jtreg/compiler/c2/irTests/TestScopedValue.java line 236:
> 
>> 234:     @IR(counts = {IRNode.LOAD_D, "1" })
>> 235:     public static double testFastPath7(boolean[] flags) {
>> 236:         double res = 0;
> 
> Suggestion:
> 
>         double res = 0;
>         // hoisted here before the loop, and commoned.
> 
> Would that be correct?

Yes, it's correct

> test/hotspot/jtreg/compiler/c2/irTests/TestScopedValue.java line 524:
> 
>> 522:         MyDouble sv1 = localSV.get();
>> 523:         notInlined();
>> 524:         MyDouble sv2 = localSV.get(); // should optimize out
> 
> Why does this work now, and some other cases with `notInlined` in between do not work?

Because the `sv` field is loaded in a local before the call.

> test/hotspot/jtreg/compiler/scoped_value/TestScopedValueBadDominatorAfterExpansion.java line 30:
> 
>> 28:  * @summary SIGSEGV in PhaseIdealLoop::get_early_ctrl()
>> 29:  * @compile --enable-preview -source ${jdk.version} TestScopedValueBadDominatorAfterExpansion.java
>> 30:  * @run main/othervm --enable-preview -XX:-BackgroundCompilation -XX:-TieredCompilation -XX:+UseParallelGC TestScopedValueBadDominatorAfterExpansion
> 
> Why is the parallel GC required here? Can you also have a run without flags, so that other GC's could be tried with this code?

I don't remember the details (I wrote this test a couple months ago) but, most likely, the G1 write barrier got in the way. I'll add a line with no gc option.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1572400445
PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1572405486
PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1572407630
PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1572393836