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

[Zoltán Majó]

Hi Vladimir,


On 07/14/2015 05:41 AM, Vladimir Kozlov wrote:
> Nice work, Zoltan

thank you!

>
> I am worry about CompilerOracle::has_option_value() call and name() == 
> vmSymbols:: checks.
> WB code does ThreadInVMfromNative transition and calls 
> is_intrinsic_available_for() in VM state.
> Compiler thread did that only when calling into CompilerOracle 
> (ciMethod::has_option_value()), otherwise it use CI. But with your 
> change compiler does not make transition - it calls 
> is_intrinsic_available_for() in Native state. It may cause problems. 
> Compiler should not access directly VM data. And, on other hand, to 
> access CI data (cySymbol) we need to set up ciEnv but WB current 
> thread is not compiler thread and the thread is already in VM state 
> anyway.

thank you for catching that problem, I did not think of it before.

>
> I thought how to solve this problem but nothing simple come up. The 
> ugly solution is to have :has_option_value() call and name() == 
> vmSymbols:: checks outside is_intrinsic_available_for(). I mean to 
> duplicate in Compile::make_vm_intrinsic() and 
> C2Compiler::is_intrinsic_available_for(). But I would like to avoid 
> cloning code.

I agree with you.

>
> An other approach is Compile::make_vm_intrinsic() can go into VM state 
> for is_intrinsic_available_for() call:
>
>   bool is_available = false;
>   {
>      VM_ENTRY_MARK;
>      methodHandle mh(THREAD, m->get_Method());
>      methodHandle ct(THREAD, method()->get_Method());
>      is_available = is_intrinsic_available_for(mh, ct, is_virtual);
>   }
>
>
> But we usually do that in CI and not in compiler code. Which means we 
> have to move the method to CI but we have Matcher calls.
>
> I would ask John's opinion on this problem.

I would prefer to keep is_intrinsic_available in compiler code (instead 
of moving it to CI) because the compiler itself is in a better position 
than the CI to determine which method it can intrinsify under which 
conditions. But I'll ask John as well what he thinks and then we can 
decide then how to proceed.

Until then, I've updated Compile::make_vm_intrinsic() to go into VM 
state as you've suggested (please see webrev below).

>
> You can simplify a little too. Methods 
> intrinsic_does_virtual_dispatch_for() and 
> intrinsic_predicates_needed_for() can get just intrinsic id. Similar 
> for methods in C1.

I changed that for both C1 and C2.

>
> There are several method->method_holder()->name() calls which could be 
> done only once.

I changed that as well.

>
> Use different name for 'method' local to avoid using Compile::
>
> +   Method* method = m->get_Method();
> +   Method* compilation_context = Compile::method()->get_Method();

I've updated the variable name.

>
> Please, add comment to new test explaining why you chose crc32 
> intrinsic. Why?

I hoped that using a single test method for all compilation levels 
tested will keep the test simple. The crc32 is intrinsic is available 
with both C1 and C2 and both intrinsics can be controlled with a single 
flag, UseCRC32Intrinsics, in both product- and fastdebug builds. I 
updated the test to clarify that.

Here is the updated webrev (for hotspot):
http://cr.openjdk.java.net/~zmajo/8130832/hotspot/webrev.01/

I tested with
- the hotspot testset with JPRT
- all hotspot/compiler JTREG tests

All tests pass.

Thank you and best regards,


Zoltan

>
> Thanks,
> Vladimir
>
> On 7/13/15 9:48 AM, Zoltán Majó wrote:
>> Hi,
>>
>>
>> please review the following patch for JDK-8130832.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8130832
>>
>> Problem: Currently, compiler-related tests frequently use the 
>> architecture descriptor string (e.g., "aarch64", "x86") to
>> determine if a certain compiler feature is available on the platform 
>> where the JVM is executing. If the tested features
>> is a compiler intrinsic, using architecture descriptor strings is an 
>> inaccurate way of determining if the intrinsic is
>> available. The reason is that the availability of compiler intrinsics 
>> is guided by many factors (e.g., the value of
>> command line flags and instructions available on the platform) and as 
>> a result a test might expect an intrinsic to be
>> available when it is in fact not available.
>>
>>
>> Solution: This enhancement proposes adding a new WhiteBox method, 
>> is_compiled_intrinsic_available(Executable method, int
>> compileLevel) that returns true if an intrinsic for method 'method' 
>> is available at compile level 'compileLevel' (the
>> final API might differ from the proposed API).
>>
>> To test the new API, a new test, 
>> hotspot/test/compiler/intrinsics/IntrinsicAvailableTest.java, is 
>> added. Moreover,
>> existing tests in hotspot/test/compiler/intrinsics/mathexact/sanity 
>> are be updated to use the newly added WhiteBox method.
>>
>> Webrev:
>> - top: http://cr.openjdk.java.net/~zmajo/8130832/top/webrev.00/
>> - hotspot: http://cr.openjdk.java.net/~zmajo/8130832/hotspot/webrev.00/
>>
>> Testing:
>> - all JPRT tests in the hotspot testset (incl. all tests in 
>> hotspot/compiler/intrinsics/mathexact), all tests pass;
>> - all hotspot JTREG tests executed locally on Linux x86_64, all tests 
>> pass that pass with an unmodified VM; all tests
>> were executed also with -Xcomp;
>> - hotspot/test/compiler/intrinsics/IntrinsicAvailableTest.java and 
>> all tests in hotspot/compiler/intrinsics/mathexact
>> executed on arm64, all tests pass;
>> - manual testing on Linux x86_64 to verify that the functionality of 
>> the DisableIntrinsic flag is preserved.
>>
>> Thank you and best regards,
>>
>>
>> Zoltan
>>