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