RFR: 8319850: PrintInlining should print which methods are late inlines [v26]

[Christian Hagedorn]

On Thu, 23 Jan 2025 15:18:37 GMT, Theo Weidmann <tweidmann at openjdk.org> wrote:

>> In https://github.com/openjdk/jdk/pull/16595 @caojoshua previously suggested changes to indicate which calls were inlined late, when printing inlines. This PR re-introduces the changes from the previously closed PR and fixes a minor issue where asserts were triggered.
>> 
>> Concerns were raised by @rwestrel in the previous PR:
>> 
>>> When InlineTree::ok_to_inline() is called, some diagnostic message is recorded for the call site. Do I understand right that with this patch, if the call is inlined late, then that message is dropped and replaced by a new "late inline.." message? If that's the case, isn't it the case that sometimes the InlineTree::ok_to_inline() has some useful information that's lost when late inlining happens?
>> 
>> As already pointed out in the PR by @caojoshua, this does not matter for string/methodhandle/vector/boxing late inlines, as they are [only performed if ok_to_inline() returns true](https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/doCall.cpp#L189). This is also the only call to ok_to_inline().  
>> 
>> The only other location, where late inline call generators are created, are calls to CallGenerator::for_late_inline_virtual(), which creates a LateInlineVirtualCallGenerator. LateInlineVirtualCallGenerator (introduced in https://github.com/openjdk/jdk/pull/1550) does not really perform inlining but rather performs strength reduction from virtual to static calls. As can be verified by running the according test `test/hotspot/jtreg/compiler/c2/irTests/TestPostParseCallDevirtualization.java`, this does not affect the printing for inlining:
>> 
>> 
>> 5022 1026       3       compiler.c2.irTests.TestPostParseCallDevirtualization::callHelper (7 bytes)   made not entrant
>>                               @ 1   compiler.c2.irTests.TestPostParseCallDevirtualization$I::method (0 bytes)   failed to inline: virtual call
>> 
>> 
>> Thus, as far as I can tell, the proposed changes by @caojoshua do not lose any useful information about why no inlining prior to the late inlining occurred.
>
> Theo Weidmann has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 35 commits:
> 
>  - Update printinlining.cpp
>  - Merge branch 'master' into 8319850
>  - Revert "Add UDivI/L and UModI/L to no_dependent_zero_check"
>    
>    This reverts commit b72ff10ba1581372b72edeadd3cf01a97ccf1c73.
>  - Revert "Update TestSplitDivisionThroughPhi.java"
>    
>    This reverts commit 7526bafff4ea26cb45894477b33f3dd24215e667.
>  - Update TestSplitDivisionThroughPhi.java
>  - Add UDivI/L and UModI/L to no_dependent_zero_check
>  - Merge branch 'master' into 8319850
>  - Fix tests
>  - Merge branch 'master' into 8319850
>  - Add missing header
>  - ... and 25 more: https://git.openjdk.org/jdk/compare/3069e912...c30910cd

Some minor comments, otherwise, looks good to me, too. Great work!

src/hotspot/share/opto/printinlining.cpp line 25:

> 23:  */
> 24: 
> 25: #include "printinlining.hpp"

Suggestion:

#include "opto/printinlining.hpp"

src/hotspot/share/opto/printinlining.hpp line 2:

> 1: /*
> 2:  * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.

Suggestion:

 * Copyright (c) 2024, 2025, Oracle and/or its affiliates. All rights reserved.

src/hotspot/share/opto/printinlining.hpp line 47:

> 45: 
> 46:   public:
> 47:     IPInlineAttempt() {}

Since this is a default constructor, you could probably use:
Suggestion:

    IPInlineAttempt() = default;

src/hotspot/share/opto/printinlining.hpp line 105:

> 103:     // If this is a new site, provide the callee otherwise null.
> 104:     // Returned pointer is valid until any at_bci is called with non-null callee.
> 105:     InlinePrinter::IPInlineSite& at_bci(int bci, ciMethod* callee);

Redundant:
Suggestion:

    IPInlineSite& at_bci(int bci, ciMethod* callee);

test/hotspot/jtreg/compiler/inlining/LateInlinePrinting.java line 2:

> 1: /*
> 2:  * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.

Suggestion:

 * Copyright (c) 2024, 2025, Oracle and/or its affiliates. All rights reserved.

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

Marked as reviewed by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/21899#pullrequestreview-2572637121
PR Review Comment: https://git.openjdk.org/jdk/pull/21899#discussion_r1928690058
PR Review Comment: https://git.openjdk.org/jdk/pull/21899#discussion_r1928672702
PR Review Comment: https://git.openjdk.org/jdk/pull/21899#discussion_r1928691656
PR Review Comment: https://git.openjdk.org/jdk/pull/21899#discussion_r1928692474
PR Review Comment: https://git.openjdk.org/jdk/pull/21899#discussion_r1928674793