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

[Vladimir Kozlov]

Pass methodHandle parameters to Compile::is_intrinsic_available_for() in C2.
CompilerOracle::has_option_value() has methodHandle parameter.

And it is safer to use methodHandle.

And you forgot 'hg add' for new test - it is not in webrev.

Thanks,
Vladimir

On 7/14/15 9:27 AM, Zoltán Majó wrote:
> 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
>>>
>