RFR: 8261137: Optimization of Box nodes in uncommon_trap [v6]

[Vladimir Ivanov]

On Fri, 5 Mar 2021 22:26:27 GMT, Vladimir Ivanov <vlivanov at openjdk.org> wrote:

>> src/hotspot/share/opto/callGenerator.cpp line 659:
>> 
>>> 657: 
>>> 658:   if (C->eliminate_boxing()) {
>>> 659:     replace_box_to_scalar(call, callprojs.resproj);
>> 
>> I suggest to lift the checks from `replace_box_to_scalar` to here. Otherwise, it's confusing to apply the method to an arbitrary call.
>> 
>> if (resproj != nullptr && call->is_CallStaticJava() &&
>>        call->as_CallStaticJava()->is_boxing_method())
>
> Another confusing aspect: what happens with `call` node after `replace_box_to_scalar` call is over?
> 
> My understanding is you rely on `replace_box_to_scalar` to eliminate all the uses by safepoints and then let  the following code to clean it up:
>   bool result_not_used = (callprojs.resproj == NULL || callprojs.resproj->outcnt() == 0);
>   if (is_pure_call() && result_not_used) {
>     // The call is marked as pure (no important side effects), but result isn't used.
>     // It's safe to remove the call.
>     GraphKit kit(call->jvms());
>     kit.replace_call(call, C->top(), true);
> 
> The relation between `is_pure_call()` and `C->eliminate_boxing()`/`call->as_CallStaticJava()->is_boxing_method()` is not clear.
> 
> I suggest to refactor `is_pure_call()` and add a special case for `is_boxing_method()` to perform scalarization when there are only debug usages. 
> 
> Also, add an assert in/after `replace_box_to_scalar` ensuring there are no users left, so the call can go away.

Regarding `C->eliminate_boxing()`: why do you think it is needed? 

I'm asking because `is_pure_call() && result_not_used` doesn't have that check.

My understanding is the code encounters `is_boxing_method()` there only when `Compile::should_delay_boxing_inlining() == true` which requires both `EliminateAutoBox` and `AggressiveUnboxing`

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

PR: https://git.openjdk.java.net/jdk/pull/2401