RFR: 8293798: Fix test bugs due to incompatibility with -XX:+AlwaysIncrementalInline

[Tobias Hartmann]

On Fri, 16 Sep 2022 13:50:02 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

> I investigated and fixed the following three files, which failed (test asserts) if they were run with `-XX:+AlwaysIncrementalInline`.
> 
> Manually tested the 3 test-files.
> I also ran some other sanity tests.
> 
> Looking forward to your feedback,
> Emanuel
> 
> **compiler/uncommontrap/Decompile.java**
> Assert triggered:
> `java.lang.RuntimeException: Wrong compilation status.: expected false to equal true`
> We expected the test to deoptimize after introducing the third class Y to a method that is expected to be call with bimorphic inlining.
> 
> Analysis:
> It seems that we don't to bimorphic inlining of function calls (2 static inlinings, uncommon_trap for others).
> https://github.com/openjdk/jdk/blob/aa7ccdf44549a52cce9e99f6569097d3343d9ee4/src/hotspot/share/opto/doCall.cpp#L269-L275
> Instead, we create two `LateInlineCallGenerator`, one for a class (hit), and one for the other cases (miss).
> https://github.com/openjdk/jdk/blob/aa7ccdf44549a52cce9e99f6569097d3343d9ee4/src/hotspot/share/opto/doCall.cpp#L200-L201
> `LateInlineCallGenerator` has `is_inline` false, so we do not go into the bimorphic inlining case (would require both receivers to have is_inline true).
> https://github.com/openjdk/jdk/blob/aa7ccdf44549a52cce9e99f6569097d3343d9ee4/src/hotspot/share/opto/doCall.cpp#L255-L259
> Instead, we generate a static call for one class (hit), and a virtual call for the rest (miss).
> https://github.com/openjdk/jdk/blob/aa7ccdf44549a52cce9e99f6569097d3343d9ee4/src/hotspot/share/opto/doCall.cpp#L279-L280
> Therefore, when the third class comes along the function does not trap, and not deopt either.
> 
> Solution:
> For now, we will just deactivate the flag in the test. But more discussion is required if we want this flag to change the inline behavior in this way.
> 
> **compiler/intrinsics/klass/CastNullCheckDroppingsTest.java**
> Assert triggered:
> `java.lang.AssertionError: compilation must not get deoptimized`
> We expect the function `testMHCast(String s)` to be compiled without a `null_check` for input `s`. We test that by calling it with `Null`. We trigger the assert because of a `null_check`, leading to deoptimization.
> 
> Analysis:
> We are in `GraphKit::gen_checkcast`. We have a `cast` that is wrapped in a `MethodHandle`, which we want to call via `invokeExact`. Normally, this would get directly inlined, and the output is deduced to be `String*`. With the flag, however, we do not inline `invokeExact`, but create a `JavaStaticCall`, which represents `invokeExact`. This has 4 `Object*` inputs and 1 `Object*` output. So now the result of the cast is a `Object*`.
> We have a `String*` in the normal case, and `Object*` in the flag case. We now do a local cast to `String*`. For that we first check subtyping.
> 
> https://github.com/openjdk/jdk/blob/aa7ccdf44549a52cce9e99f6569097d3343d9ee4/src/hotspot/share/opto/graphKit.cpp#L3311
>     In the normal case, we have a perfect match - so we rely on profiling and do speculative `String::NonNull*`. But in the flag case, we see `Object*` is not subtype of `String*`. So we continue on, and find that profiling has determined `never_see_null = true`, so we set an `uncommon_trap` for the `null_check`.
> 
> Somehow, the speculative profiling case does not lead to a trap - not sure why yet.
> Of course in the test we eventually do feed in `Null` - in the regular case there is no trap, in the flag case we trap and deopt, breaking the test assumption.
> 
> Solution:
> At any rate, this really looks like a test bug - the flag can change the decisions that have an impact on the asserts of the test. I will disable the flag.
> 
> **compiler/ciReplay/TestInliningProtectionDomain.java**
> Assert triggered:
> `java.lang.RuntimeException: assertTrue: expected true, was false`
> https://github.com/openjdk/jdk/blob/aa7ccdf44549a52cce9e99f6569097d3343d9ee4/test/hotspot/jtreg/compiler/ciReplay/TestInliningProtectionDomain.java#L73
> 
> Analysis:
> Turns out `isForcedByReplay()` returns false because the "reason" is expected to be `force inline by ciReplay`, but we get a reason `force (incremental) inline by ciReplay`. This seems expected. We should use `isForcedIncrementalInlineByReplay()` if we expect incremental inline to happen.
> 
> Solution:
> `isForcedByReplay()` or `isForcedIncrementalInlineByReplay()`

Looks good.

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

Marked as reviewed by thartmann (Reviewer).

PR: https://git.openjdk.org/jdk/pull/10310