[XXS] C1 misses to dump a reason when it inlines successfully

[Tobias Hartmann]

Hi,

looks good to me too.

Best regards,
Tobias

On 29.11.19 09:51, Doerr, Martin wrote:
> Hi,
> 
>  
> 
> looks good to me. Thanks for cleaning it up.
> 
>  
> 
> Best regards,
> 
> Martin
> 
>  
> 
>  
> 
> *From:*Liu Xin <navy.xliu at gmail.com>
> *Sent:* Freitag, 29. November 2019 08:17
> *To:* Doerr, Martin <martin.doerr at sap.com>
> *Cc:* Tobias Hartmann <tobias.hartmann at oracle.com>; hotspot-compiler-dev at openjdk.java.net
> *Subject:* Re: [XXS] C1 misses to dump a reason when it inlines successfully
> 
>  
> 
> Hi, Tobias & Martin, 
> 
>  
> 
> The original message even didn't say inline succeed or not. When I see those, I have to dig into the
> source code. I spent even more time because I didn't realize that those messages are from C1. That's
> my standpoint and motivation as a new hotspot developer.
> 
> What C2 inliner emits "inline (hot)"  is not very informative neither, but at least developers know
> that this callee is inlined. I gave up "by the rules of C1". I think "inline" is fine. developers
> can also tell difference from C2's output. 
> 
>  
> 
> I agree with you about the default parameter.  Failed attempts should always have a reason.
> Developers can inspect them and make adjustment.
> 
>  
> 
> Here is a new revision. Could you take a look?
> 
> https://cr.openjdk.java.net/~xliu/8234541/01/webrev/
> 
>  
> 
> sample output is like this.  
> 
>                               @ 1   java.lang.String::isLatin1 (19 bytes)   inline
>                               @ 12   java.lang.StringLatin1::charAt (28 bytes)   inline
>                                 @ 15  java/lang/StringIndexOutOfBoundsException::<init> (not loaded)
>   not inlineable
>                               @ 21  java/lang/StringUTF16::charAt (not loaded)   not inlineable
>                               @ 15  java/lang/StringIndexOutOfBoundsException::<init> (not loaded)  
> not inlineable
> 
>                               @ 3   java.util.stream.ReduceOps::makeInt (18 bytes)   inline
>                                 @ 1   java.util.Objects::requireNonNull (14 bytes)   inline
>                                   @ 8   java.lang.NullPointerException::<init> (5 bytes)   don't
> inline Throwable constructors
>                                 @ 14   java.util.stream.ReduceOps$6::<init> (16 bytes)   inline
>                                   @ 12   java.util.stream.ReduceOps$ReduceOp::<init> (10 bytes)   inline
>                                     @ 1   java.lang.Object::<init> (1 bytes)   inline
>                               @ 6   java.util.stream.AbstractPipeline::evaluate (94 bytes)   callee
> is too large
> 
> thanks, 
> 
> --lx
> 
>  
> 
>  
> 
>  
> 
> On Thu, Nov 28, 2019 at 6:53 AM Doerr, Martin <martin.doerr at sap.com <mailto:martin.doerr at sap.com>>
> wrote:
> 
>     Hi,
> 
>     I agree with Tobias. " by the rules of C1" doesn't add any useful information IMHO.
>     But I'd be fine with just adding "inline" to avoid the confusion.
> 
>     > Anyway, with your change, all callers now pass a msg argument and
>     > therefore the null handling logic
>     > should be removed (replaced by an assert). Also, the default value for the
>     > argument can be removed.
>     +1
> 
>     Best regards,
>     Martin
> 
> 
>     > -----Original Message-----
>     > From: hotspot-compiler-dev <hotspot-compiler-dev-
>     > bounces at openjdk.java.net <mailto:bounces at openjdk.java.net>> On Behalf Of Tobias Hartmann
>     > Sent: Donnerstag, 28. November 2019 15:04
>     > To: Liu Xin <navy.xliu at gmail.com <mailto:navy.xliu at gmail.com>>;
>     hotspot-compiler-dev at openjdk.java.net <mailto:hotspot-compiler-dev at openjdk.java.net>
>     > Subject: Re: [XXS] C1 misses to dump a reason when it inlines successfully
>     >
>     > Hi,
>     >
>     > I'm still not convinced that this message adds any useful information but let's
>     > see what other
>     > reviewers think.
>     >
>     > Anyway, with your change, all callers now pass a msg argument and
>     > therefore the null handling logic
>     > should be removed (replaced by an assert). Also, the default value for the
>     > argument can be removed.
>     >
>     > Best regards,
>     > Tobias
>     >
>     > On 22.11.19 19:09, Liu Xin wrote:
>     > > hi, Reviewers,
>     > >
>     > > Could you review this extremely small change?
>     > > Bugs: https://bugs.openjdk.java.net/browse/JDK-8234541
>     > > Webrev: https://cr.openjdk.java.net/~xliu/8234541/00/webrev/
>     > >
>     > > When I analyzed PrintInlining, I was confused by the inline message
>     > without
>     > > any detail. It's not easy for developer to tell if this method is inlined
>     > > or not. This patch add a comment "inline by the rules of C1".
>     > >
>     > > I would like to add an explicit reason, but there's no decisive reason in
>     > > GraphBuilder::try_inline_full. It just passes all restrict rules. Any other
>     > > suggestion would be appreciated.
>     > >
>     > > Thanks,
>     > > --lx
>     > >
>