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

[Vladimir Kozlov]

Nice work, Zoltan

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.

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.

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.

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.

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

Use different name for 'method' local to avoid using Compile::

+   Method* method = m->get_Method();
+   Method* compilation_context = Compile::method()->get_Method();

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

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
>