RFR(S): Remove faulty assertion in Compile::call_generator triggered with -XX:+AlwaysIncrementalInline

[Ludovic Henry]

Ok, let me squash the changes together and upload it. Thank you for looking into it.

--
Ludovic

-----Original Message-----
From: Tobias Hartmann <tobias.hartmann at oracle.com> 
Sent: Wednesday, June 17, 2020 3:21 AM
To: Ludovic Henry <luhenry at microsoft.com>; hotspot-compiler-dev at openjdk.java.net
Subject: Re: RFR(S): Remove faulty assertion in Compile::call_generator triggered with -XX:+AlwaysIncrementalInline

Hi Ludovic,

this is only an issue with your patch that removes 'delayed_forbidden' (see my reply in that
thread). I would therefore suggest to remove the assert with that patch.

Best regards,
Tobias

On 17.06.20 07:16, Ludovic Henry wrote:
> Hi,
> 
> While playing with -XX:+AlwaysIncrementalInline, I ran into an assertion triggering on pretty much any tests in test/hotspot/jtreg in debug (slow or fast, assertion just need to be enabled). I tracked it down to the following issue and explanation.
> 
> In CallGenerator, when calling `for_method_handle_call`, it will redirect to one of the following:
> - `for_method_handle_inline` itself redirecting to `Compile::call_generator`
> - `for_late_inline`
> - `for_mh_late_inline`
> - `for_direct_call`
> 
> The removed assert would fail if we end up calling `for_late_inline`. That would be the case when `for_method_handle_inline` returns successfully and AlwaysIncrementalInline is set. This would guarantee failure whenever this global setting is set with -XX:+AlwaysIncrementalInline
> 
> In this case, we can safely remove the assert because the case treated specificically in `for_mh_late_inline` is treated by the combination of `for_method_handle_inline` and `for_late_inline`.
> 
> Looking at `LateInlineMHCallGenerator::do_late_inline_check`, which is called from `LateInlineCallGenerator::do_late_inline`, we notice that it itself calls `for_method_handle_inline`. This means that `for_method_handle_inline` is called through the two following path with late inlining:
> 1. `for_late_inline` -> `do_late_inline` <- `for_method_handle_inline` (which was previously called and its result passed through `for_late_inline(inline_cg)`)
> 2. `for_late_mh_inline` -> `do_late_inline` -> `do_late_inline_check` -> `for_method_handle_inline`
> 
> So whether we return a LateInlineCallGenerator or a LateInlineMHCallGenerator from `for_method_handle_call`, the logic in `for_method_handle_inline` will be called.
> 
> I do not have authorship status, so I can neither create a JBS nor a webrev. I am working with a colleague to do it, and I'll link it here, but feel free to create any of it before that. The diff is available at [1]
> 
> Thank you,
> 
> --
> Ludovic
> 
> [1]
> diff --git a/src/hotspot/share/opto/doCall.cpp b/src/hotspot/share/opto/doCall.cpp
> index c4d55d0d4c4..ba5f737592e 100644
> --- a/src/hotspot/share/opto/doCall.cpp
> +++ b/src/hotspot/share/opto/doCall.cpp
> @@ -146,7 +146,6 @@ CallGenerator* Compile::call_generator(ciMethod* callee, int vtable_index, bool
>    // have bytecodes and so normal inlining fails.
>    if (callee->is_method_handle_intrinsic()) {
>      CallGenerator* cg = CallGenerator::for_method_handle_call(jvms, caller, callee);
> -    assert(cg == NULL || !cg->is_late_inline() || cg->is_mh_late_inline(), "unexpected CallGenerator");
>      return cg;
>    }
> 
>