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

[Zoltán Majó]

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::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

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::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.

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 is_intrinsic_available[_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::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?).

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 
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 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/

Testing:
- 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.

Thank you!

Best regards,


Zoltan


>
> — John
>