8183232: Avoid resolving method_kind in AbstractInterpreter::can_be_compiled

[Doerr, Martin]

Hi Claes,

you can consider my mail as review. It'd only be nice to improve comments a little bit. The intention behind can_be_compiled() is not really clear from the comments.
I'd vote for pushing your change as it is and rewrite can_be_compiled() in the larger scope of JDK-8183232.

Best regards,
Martin


-----Original Message-----
From: Claes Redestad [mailto:claes.redestad at oracle.com] 
Sent: Freitag, 30. Juni 2017 14:13
To: Doerr, Martin <martin.doerr at sap.com>; hotspot-runtime-dev at openjdk.java.net; 'hotspot-compiler-dev at openjdk.java.net' <hotspot-compiler-dev at openjdk.java.net>
Subject: Re: 8183232: Avoid resolving method_kind in AbstractInterpreter::can_be_compiled

Hi Martin,

On 06/30/2017 12:14 PM, Doerr, Martin wrote:
> Hi Claes,
>
> adding hotspot-compiler mailing list since it is JIT related.
>
> First of all, thanks for addressing this performance issue. I think it makes sense to avoid method resolution (method_kind()).
>
> I'm basically ok with the change, but I'd like to share some thoughts.

thanks for reviewing(?)!

>
> I think the intention of can_be_compiled() is to prevent compilation of floating point related methods in a stand-alone fashion. The C2 compiler wouldn't use intrinsics in this case. When e.g. C2 inlines the same method into some caller, the intrinsics will be used and may result in a slightly different floating point computation as the stand-alone compiled method (for non-strict math). So the purpose is to enforce consistent computation between interpreter and all compiled methods.

Right.

>
> Your current proposal just disables stand-alone computation for all math intrinsics. This still serves the purpose. I guess it wouldn't even really hurt, to prevent stand-alone compilation of intrinsics in general.
> But it seems like the design goal was to prevent stand-alone compilation of exactly these functions, which have special compiler intrinsics. This is platform dependent.
> I'm not requesting a change, but I just want to mention that the change kind of breaks this design.

Shall I count this as a vote in favor of keeping the platform specific 
implementations of can_be_compiled? I have no strong opinion against 
reverting back to that, just that there'll be more places to change 
going forwards. It just seemed to serve little practical purpose on the 
set of platforms I've tested right now.

/Claes

>
> Please also note, that there's already an open bug to improve math intrinsics related logic:
> https://bugs.openjdk.java.net/browse/JDK-8170693
> Maybe can_be_compiled() can get replaced in that context.
>
> Best regards,
> Martin
>
>
> -----Original Message-----
> From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-bounces at openjdk.java.net] On Behalf Of Claes Redestad
> Sent: Donnerstag, 29. Juni 2017 18:13
> To: hotspot-runtime-dev at openjdk.java.net
> Subject: RFR: 8183232: Avoid resolving method_kind in AbstractInterpreter::can_be_compiled
>
> Hi all,
>
> it appears profitable for startup to simplify
> AbstractInterpreter::can_be_compiled
> to use methodHandle->intrinsic_id() rather than take a detour through
> method_kind.
>
> webrev: http://cr.openjdk.java.net/~redestad/8183232/hotspot.00/
> bug: https://bugs.openjdk.java.net/browse/JDK-8183232
>
> Note: Having platform-dependent implementations for can_be_compiled
> seems excessive,
> since if the intrinsic isn't implemented the corresponding case will
> never trigger anyway.
> No regression can be detected on platforms that doesn't have certain
> intrinsics, such as
> SPARC, but if there's preference I can revert to having
> platform-dependent methods.
>
> Testing: JPRT
>
> Thanks!
>
> /Claes