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

[Vladimir Kozlov]

Looks good to me. We still need John's opinion about state transition.

Thanks,
Vladimir

On 7/15/15 11:15 AM, Zoltán Majó wrote:
> Hi Vladimir,
>
>
> On 07/14/2015 07:11 PM, Vladimir Kozlov wrote:
>> 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.
>
> OK, I updated the method.
>
>>
>> And you forgot 'hg add' for new test - it is not in webrev.
>
> Sorry for that. The test is included into the newest webrev:
> - top: http://cr.openjdk.java.net/~zmajo/8130832/top/webrev.02/
> - hotspot: http://cr.openjdk.java.net/~zmajo/8130832/hotspot/webrev.02/
>
> I ran all JPRT tests from the hotspot testset, all pass.
>
> Thank you and best regards,
>
>
> Zoltan
>
>>
>> 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
>>>>>
>>>
>