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

Zoltán Majó zoltan.majo at oracle.com
Tue Jul 21 15:19:26 UTC 2015

Hi John,

thank you for the feedback!

On 07/15/2015 09:11 PM, John Rose wrote:

On Jul 15, 2015, at 11:44 AM, Vladimir Kozlov <vladimir.kozlov at oracle.com <mailto: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::makevmintrinsic() [2] and it works well. What we could also do is - (1) list in the switch statement in isintrinsicavailablefor() 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::hasoptionvalue()we could define a separate method that is called directly from a WhiteBox context and through the CI from makevmintrinsic. 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

I removed the "_for" suffix from relevant method names.

It seems that the style guide does not list "_for" as a noise word. I tried to update the page, but I don't seem to have the necessary access rights.

I agree with the tendency to factor stuff (when possible) away from the guts of the compilers. Suggest Compile::intrinsicdoesvirtualdispatchfor be moved to vmIntrinsics::doesvirtualdispatch. It's really part of the vmIntrinsics contract. Same for cantrap (or whatever it is called). If it can't be wedged into vmSymbols.cpp, then at least consider abstractCompiler.cpp.

I relocated the following methods:

Compile::intrinsic_does_virtual_dispatch_for -> vmIntrinsics::does_virtual_dispatch Compile::intrinsic_predicates_needed_for -> vmIntrinsics::predicates_needed GraphBuilder::intrinsic_can_trap-> vmIntrinsics::can_trap GraphBuilder::intrinsic_preserves_state -> vmIntrinsics::preserves_state

Similar comment about isintrinsicavailable[for]. Because of the dependency on the compiler tier, it has to be virtual, of course.

OK, I changed the name of AbstractCompiler::is_intrinsic_available_for to AbstractCompiler::is_intrinsic_available. The method is virtual and is overridden by Compiler and by C2Compiler.

Suggest a static vmIntrinsics::isdisabledbyflags, to check for compiler-independent disabling logic. Method::isintrinsicdisabled 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 intrinsicid. Stripping the Method* would let you avoid a VMENTRYMARK in ciMethod::* if the context argument if null (true for C1?).

I managed to extract most of the flag-disabling logic (the parts common to C1 and C2) to the vmIntrinsics::is_disabled_by_flags static method.

The compiler-specific parts are implemented by the hierarchy starting at AbstractCompiler::is_intrinsic_disabled_by_flag. Some of the compiler-specific flag-disabling logic might be also considered as an inconsistency between C1 and C2 (please see below).

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 vmversion 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.)

I factored the Matcher calls (for C2) and the vm_version checks (for C1) into the hierarchy starting at AbstractCompiler::is_intrinsic_supported().

Regarding the compiler-specific flag-disabling logic, we can make the following observations:

  1. The DisableIntrinsic flag is C2-specific therefore it is currently included in C2Compiler::is_intrinsic_disabled_by_flag.

  2. The InlineNatives flag disables most but not all intrinsics. There are some intrinsics (implemented by both C1 and C2) but that -XX:-InlineNatives turns off for C1 but leaves unaffected for C2.

  3. The _getClass intrinsic (implemented by both C1 and C2) is turned off by -XX:-InlineClassNatives for C1 and is left unaffected by C2.

  4. The _loadfence, _storefence, _fullfence, _compareAndSwapObject,
    _compareAndSwapLong, and _compareAndSwapInt intrinsics are turned off by -XX:-InlineUnsafeOps for C2 and are unaffected by C1.

Compiler-specific functionality related to observations (1)-(4) is currently implemented in the hierarchy starting at AbstractCompiler::is_intrinsic_disabled_by_flag. If we decide to standardize some parts of flag processing, we can move the relevant functionality to vmIntrinsics::is_disabled_by_flag().

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

Thank you for clarifying! Currently, C1 can perform all checks without going into VM mode. For C2, only vmIntrinsics::is_disabled_by_flags() can be executed in native mode, it seems that the rest of the checks needed by C2 must be performed in VM mode. The logic in Compile::make_vm_intrinsic reflects these considerations.

Here is the newest webrev:

Testing:

Thank you!

Best regards,

Zoltan

— John

More information about the hotspot-compiler-dev mailing list