RFR(M): 8130832: Extend the WhiteBox API to provide information about the availability of compiler intrinsics
zoltan.majo at oracle.com
Tue Jul 21 15:19:26 UTC 2015
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::make_vm_intrinsic()  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:
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
> 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
> 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
I relocated the following methods:
Compile::intrinsic_predicates_needed_for -> vmIntrinsics::predicates_needed
GraphBuilder::intrinsic_preserves_state -> vmIntrinsics::preserves_state
> Similar comment about is_intrinsic_available[_for]. Because of the
> 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::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
> 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 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.)
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
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 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.
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:
- top: http://cr.openjdk.java.net/~zmajo/8130832/top/
- hotspot: http://cr.openjdk.java.net/~zmajo/8130832/hotspot/webrev.03/
- all JTREG tests run locally (linux-x86_64), all pass;
- all JPRT tests (testset hotspot, including the newly added test,
compiler/intrinsics/IntrinsicAvailableTest.java), all tests pass;
- all tests in hotspot/test/compiler/intrinsics/mathexact on aarch64,
all tests pass.
> — John
More information about the hotspot-compiler-dev