[9] RFR(M): 8130832: Extend the WhiteBox API to provide information about the availability of compiler intrinsics

[John Rose]

On Jul 15, 2015, at 11:44 AM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
> 
> Looks good to me. We still need John's opinion about state transition.

Just sent a 1-1 reply; here it is FTR.

On Jul 14, 2015, at 9:42 AM, Zoltán Majó <zoltan.majo at oracle.com <mailto:zoltan.majo at oracle.com>> wrote:
> 
> So far, I tried Vladimir's solution of going into VM state in Compile::make_vm_intrinsic() [2] and it works well.
> 
> What we could also do is
> - (1) list in the switch statement in is_intrinsic_available_for() the intrinsic ids of all methods of interest (similarly to the way we do it for C1); that would eliminate the need to make checks based on a method's holder;
> - (2) for the DisableIntrinsic checks (that need to call CompilerOracle::has_option_value()we could define a separate method that is called directly from a WhiteBox context and through the CI from make_vm_intrinsic.

This is going to be a good cleanup.  But it is hard to follow, so please
regard my comments as tentative.

Some comments:

I think the term "_for" is a noise word as deprecated in:
 https://wiki.openjdk.java.net/display/HotSpot/StyleGuide#StyleGuide-NamingNaming <https://wiki.openjdk.java.net/display/HotSpot/StyleGuide#StyleGuide-NamingNaming>

I agree with the tendency to factor stuff (when possible) away from the guts of the compilers.

Suggest Compile::intrinsic_does_virtual_dispatch_for be moved to vmIntrinsics::does_virtual_dispatch.
It's really part of the vmIntrinsics contract.  Same for can_trap (or whatever it is called).
If it can't be wedged into vmSymbols.cpp, then at least consider abstractCompiler.cpp.

Similar comment about is_intrinsic_available[_for].  Because of the dependency
on the compiler tier, it has to be virtual, of course.  Suggest a static
vmIntrinsics::is_disabled_by_flags, to check for compiler-independent disabling logic.
Method::is_intrinsic_disabled is a good thought, but I would suggest making it
a static method on vmIntrinsic, because the Method* pointer is just a wrapper around
the intrinsic_id.  Stripping the Method* would let you avoid a VM_ENTRY_MARK
in ciMethod::* if the context argument if null (true for C1?).

The "flag soup" logic in C2 is frustrating, and may defeat an attempt to factor
the -XX flag checking into vmIntrinsics, but I encourage you to try.
The Matcher calls can be layered on separately, in C2-specific code.
The vm_version checks can go in the same C1/C2-specific layer
as the C2 matcher checks.  (Or perhaps factored into abstractCompiler,
but that may be overkill.)

Regarding your original question:  I would prefer that the VM_ENTRY
logic be confined to the CI, but there is no functional reason the
compiler itself can't do a native-to-VM transition.

— John

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20150715/25b686a2/attachment-0001.html>