[lworld] RFR: 8263024: Convert Valhalla tests using the old framework to the new framework [v2]

Tue Jul 13 08:45:17 UTC 2021

On Tue, 13 Jul 2021 06:24:39 GMT, Ekaterina Pavlova <epavlova at openjdk.org> wrote:

>> This PR converts compiler/valhalla/inlinetypes tests based on old Valhalla/IR test framework to new one 
>> which was recently integrated by Christian as part of JDK-8254129.
>> The tests which were we decided to convert are basically the ones which extends InlineTypeTest class.
>> The rest of compiler/valhalla/inlinetypes tests were agreed (with Christian and Tobias) to keep untouched 
>> as they are in most cases regression tests.
>> 
>> The conversion rules/approach is pretty well described in the bug's description section (see JDK-8263024).
>> A detailed description of new IR test framework could found in test/hotspot/jtreg/compiler/lib/ir_framework/README.md.
>> 
>> Testing:
>>  Ran compiler/valhalla/inlinetypes in all the configurations used in hs-tier1-9.
>>  There 2 failed converted tests:
>>   compiler/valhalla/inlinetypes//TestNullableArrays.java - new bug JDK-8269070 filed
>>   compiler/valhalla/inlinetypes/TestIntrinsics.java - know issue tracked by JDK-8239003
>> 
>> 
>> Big thanks to Christian and Tobias for advice and help during this tests conversion.
>> 
>> Please review the changes.
>> 
>> Thanks,
>> Katya
>
> Ekaterina Pavlova has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains two commits:
> 
>  - Merge lworld
>  - Converted to new IR framework

Thanks a lot for your help to do the conversions! They look good overall. There are only some minor things, mostly about code style.

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestArrays.java line 979:

> 977:     }
> 978: 
> 979: 

New line can be removed

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestArrays.java line 1369:

> 1367:     }
> 1368: 
> 1369:     @Test

Introduced whitespace can be removed

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestBasicFunctionality.java line 58:

> 56: 
> 57:     // Helper methods
> 58: 

Maybe leave that line as the comment is about multiple methods

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestCallingConvention.java line 119:

> 117: 
> 118:     @Run(test = "test2")
> 119:     public void test2_verifier(RunInfo info) {

`RunInfo info` can be removed

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestCallingConvention.java line 372:

> 370:     }
> 371: 
> 372: 

New line can be removed.

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestCallingConvention.java line 395:

> 393:             TestFramework.assertCompiledByC2(helper_m);
> 394:         }
> 395: 

New line can be removed

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestCallingConvention.java line 517:

> 515: 
> 516:     @Run(test = "test23")
> 517:     public void test23_verifier(RunInfo info) {

`RunInfo info` can be removed.

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestCallingConvention.java line 537:

> 535: 
> 536:     @Run(test = "test24")
> 537:     public void test24_verifier(RunInfo info) {

`RunInfo info` can be removed.

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestCallingConvention.java line 1030:

> 1028:     }
> 1029: 
> 1030: 

New line can be removed

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestCallingConventionC1.java line 129:

> 127: 
> 128:         @ForceCompile(compLevel = C1)
> 129:         @DontInline

`DontInline` should not have been removed

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestCallingConventionC1.java line 208:

> 206:         int field = 1000;
> 207: 
> 208:         @DontInline @ForceCompile(compLevel = C1)

`DontInline` should not have been removed.

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestCallingConventionC1.java line 209:

> 207:         public int func1(int a, int b) { return field + a + b + 300; }
> 208: 
> 209:         @DontInline @ForceCompile(CompLevel.C1_SIMPLE)

`ForceCompile` can be put on a new line.

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestCallingConventionC1.java line 210:

> 208:         @DontInline @ForceCompile(compLevel = C1)
> 209:         public int func1(int a, int b)             { return field + a + b + 20; }
> 210:         @DontInline @ForceCompile(compLevel = C1)

`DontInline` should not have been removed.

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestCallingConventionC1.java line 217:

> 215:         int field = 2000;
> 216: 
> 217:         @DontInline @ForceCompile(compLevel = C2)

`DontInline` should not have been removed.

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestCallingConventionC1.java line 219:

> 217:         @DontInline @ForceCompile(compLevel = C2)
> 218:         public int func1(int a, int b)             { return field + a + b + 20; }
> 219:         @DontInline @ForceCompile(compLevel = C2)

`DontInline` should not have been removed.

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestCallingConventionC1.java line 222:

> 220:         public int func1(int a, int b)             { return field + a + b + 300; }
> 221: 
> 222:         @DontInline @ForceCompile(CompLevel.C2)

`ForceCompile` can be put on a new line.

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestCallingConventionC1.java line 2263:

> 2261: 
> 2262:     @ForceCompile
> 2263:     public void test107_verifier(RunInfo info) {

`RunInfo info` can be removed.

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestCallingConventionC1.java line 2287:

> 2285:     @Run(test = "test107")
> 2286:     @Warmup(0)
> 2287:     public void run_test107_verifier(RunInfo info) {

`RunInfo info` can be removed.

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestCallingConventionC1.java line 2298:

> 2296: 
> 2297:     @ForceCompile
> 2298:     public void test108_verifier(RunInfo info) {

`RunInfo info` can be removed.

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestCallingConventionC1.java line 2322:

> 2320:     @Run(test = "test108")
> 2321:     @Warmup(0)
> 2322:     public void run_test108_verifier(RunInfo info) {

`RunInfo info` can be removed.

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestCallingConventionC1.java line 2333:

> 2331: 
> 2332:     @ForceCompile
> 2333:     public void test109_verifier(RunInfo info) {

`RunInfo info` can be removed.

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestCallingConventionC1.java line 2357:

> 2355:     @Run(test = "test109")
> 2356:     @Warmup(0)
> 2357:     public void run_test109_verifier(RunInfo info) {

`RunInfo info` can be removed.

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestIntrinsics.java line 90:

> 88:     // Verify that Class::isAssignableFrom checks with statically known classes are folded
> 89:     @Test
> 90:     @IR(failOn = {LOADK})

Can be simplified to `failOn  = LOADK`.

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestIntrinsics.java line 126:

> 124:     // Verify that Class::getSuperclass checks with statically known classes are folded
> 125:     @Test
> 126:     @IR(failOn = {LOADK})

Can be simplified to `failOn = LOADK`.

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestIntrinsics.java line 384:

> 382: 
> 383:     @Test
> 384:     @IR(failOn = {CALL_Unsafe})

Can be simplified to `failOn = CALL_Unsafe`. There are some more of them below which could be simplified as well.

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestLWorld.java line 3478:

> 3476:     // acmp doesn't need substitutability test when one input null
> 3477:     @Test
> 3478:     @IR(failOn = {SUBSTITUTABILITY_TEST})

Can be simplified to `failOn = SUBSTITUTABILITY_TEST`.

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestLWorldProfiling.java line 396:

> 394:     }
> 395: 
> 396: 

New line can be removed.

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestLWorldProfiling.java line 569:

> 567:     // branch frequency profiling causes not equal branch to be optimized out
> 568:     @Test
> 569:     @IR(failOn = {SUBSTITUTABILITY_TEST})

Can be simplified to `failOn = SUBSTITUTABILITY_TEST`. There are some more of them below which could be simplified as well.

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestNullableArrays.java line 124:

> 122:     // updating its elements in a loop and computing a hash.
> 123:     @Test
> 124:     @IR(failOn = {ALLOCA})

Can be simplified to `failOn = ALLOCA`.

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestNullableArrays.java line 2567:

> 2565:     // Test loads from vararg arrays
> 2566:     @Test
> 2567:     @IR(failOn = {LOAD_UNKNOWN_INLINE})

Can be simplified to `failOn = LOAD_UNKNOWN_INLINE`.

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestUnloadedInlineTypeField.java line 46:

> 44: 
> 45: public class TestUnloadedInlineTypeField {
> 46:     // The test only prevent loading of classes when testing with C1. Load classes

`The test only` -> `Only`?

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestUnloadedInlineTypeField.java line 882:

> 880: 
> 881:     @Run(test = "test20")
> 882:     public void test20_verifier(RunInfo info) {

`RunInfo info` can be removed.

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

Changes requested by chagedorn (no project role).

PR: https://git.openjdk.java.net/valhalla/pull/457