[lworld] RFR: 8343423: [lworld] compiler/valhalla/inlinetypes/TestArrays.java IR mismatches after after merging JDK-8334060 in jdk-24+18 [v3]

[Tobias Hartmann]

On Thu, 7 Nov 2024 10:20:41 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> There are several events that need to be taken into consideration to understand why `TestArray::test97-100()` are now failing after the merge of JDK-24+18:
>> 
>> ### Background
>> #### Original Intent of `test95-101()`:
>> Orignally, the tests were added with [JDK-8251442](https://bugs.openjdk.org/browse/JDK-8251442) to check that sub type checks are properly folded with a new regex `CHECKCAST_ARRAY`. Back there, we did not have the possibility to match on compilation phases. We could only match on the output of `PrintIdeal` or `PrintOptoAssembly`. `PrintIdeal` is after the macro node expansion and thus `SubTypeCheck` nodes cannot be found anymore. As an alternative, a matching on `PrintOptoAssembly` was chosen by matching multiple lines.
>> 
>> Note that only `test101()` is a positive test for a sub type check (it requires that one is found) while `test95-100()` are negative tests that checks the absense of a sub type check.
>> 
>> #### Matching on Multiple Lines in Opto Assembly Is Fragile
>> As seen in the past, matching multiples lines on Opto Assembly is fragile when some unrelated instructions are inserted in between (for example spills). The regexes can become quite complicated and also slow to process.
>> 
>> #### `CHECKCAST_ARRAY` Stopped Working Correctly at some Point Due to Scheduling Bool and If Apart
>> At some point, code emission started to schedule the nodes for the bool/comparison earlier, then scheduled an unrelated If and only then the actual jump for the sub type check. Example:
>> 
>> ![image](https://github.com/user-attachments/assets/bc9fb4b9-3493-48a1-8c0a-f3cb5a22808f)
>> 
>> The nodes for `74 compP_reg` are first emitted. Then we emit `101 testL_reg` together with `100 jmpCon_short` and only then `73 jmpConU`:
>> 
>> --- 74 compP_rReg nodes ---
>> 0c4     B7: #	out( B9 B8 ) <- in( B32 B21 B19 B6 )  Freq: 0.87277
>> 0c4     # checkcastPP of RBP
>> 0c4     movl    R11, [RBP + #8 (8-bit)]	# compressed klass ptr
>> 0c8     encode_heap_oop_not_null R8,RBX
>> 10d     movl    [R13], R8	# compressed ptr
>> 111     movq    R10, R13	# ptr -> long
>> 114     movq    R8, RBX	# ptr -> long
>> 117     xorq    R8, R10	# long
>> 11a     shrq    R8, #22
>> 11e     movq    RBX, precise [precise compiler/valhalla/inlinetypes/MyValue1: 0x00006ffd384d65a8 (compiler/valhalla/inlinetypes/MyInterface):Constant:exact * (java/lang/Cloneable,java/io/Serializable): :Constant:exact *	# ptr
>> 128     decode_and_move_klass_not_null R9,R11
>> 12b     movq    RBP, [R9 + #80 (8-bit)]	# cl...
>
> Christian Hagedorn has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Update comment again

Nice analysis Christian! Thanks for the detailed description, this will be useful for future reference.

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

> 2421:         // At runtime, we will hit the ArrayStoreException in the first execution when array is a MyValue1[].
> 2422:         // With the default IR framework warm-up, we will profile the ArrayStoreException in the interpreter and
> 2423:         // pass it in the MDO to the C2 compiler which treat these exceptions as traps being hit (see

Suggestion:

        // pass it in the MDO to the C2 compiler which treats these exceptions as traps being hit (see

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

> 2425:         // type Object[] to an exact type before the first sub type check because we've seen too many traps being taken
> 2426:         // at that bci due to the ArrayStoreException that was hit at the very same bci (see Compile::too_many_traps()
> 2427:         // which checks that zero traps have been taken  so far). Thus, neither the first sub type check for the array

Suggestion:

        // which checks that zero traps have been taken so far). Thus, neither the first sub type check for the array

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

> 2428:         // check cast nor the second sub type check for the instanceof can be removed.
> 2429:         // By not executing test97() with MyValue1[] during warm-up, which would trigger the ArrayStoreException,
> 2430:         // we will not observe an ArrayStoreException before C2 compilation. Note that C2 also require

Suggestion:

        // we will not observe an ArrayStoreException before C2 compilation. Note that C2 also requires

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

Marked as reviewed by thartmann (Committer).

PR Review: https://git.openjdk.org/valhalla/pull/1296#pullrequestreview-2420553021
PR Review Comment: https://git.openjdk.org/valhalla/pull/1296#discussion_r1832433007
PR Review Comment: https://git.openjdk.org/valhalla/pull/1296#discussion_r1832433451
PR Review Comment: https://git.openjdk.org/valhalla/pull/1296#discussion_r1832433907