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

[Vladimir Kozlov]

WB method isIntrinsicAvailableForMethod0 has parameter 
compilationContext. So I don't think you need 
is_intrinsic_available(methodHandle method) variant - it is not used 
from WB.

You left is_intrinsic_available*_for* in comments in abstractCompiler.hpp

Following the same naming logic I would suggest to remove "ForMethod" in 
WB new methods names.

Missing 'virtual' in c2compiler.hpp

Leftover line in c2compiler.cpp:

+   //return Compile::is_intrinsic_available(method, 
compilation_context, false);

Indention is off - keep following lines at the same level as 
!compilation_context (one space after "("):

+       (!compilation_context.is_null() &&
+           CompilerOracle::has_option_value(compilation_context, 
"DisableIntrinsic", disable_intr) &&
+           strstr(disable_intr, vmIntrinsics::name_at(id)) != NULL)

Remove 'return false' because following InlineUnsafeOps check disable 
some intrinsics:

+     case vmIntrinsics::_Reference_get:
+       return false;
+       break;

Also I would prefer vmIntrinsics::is_disabled_by_flags() was called from 
compiler's is_intrinsic_disabled_by_flag() flag.

Additionally I think we should not have difference in behavior of 
is_intrinsic_disabled_by_flag() in C1 and C2. Both compilers should 
follow the same rules. If we disable intrinsic on command line - C1 
should not intrinsify it. The only difference should be in supported 
intrinsics. If you think it is big change - file an other bug (it is 
bug, not rfe) to fix it separately.

Thanks,
Vladimir

On 7/22/15 12:59 AM, Zoltán Majó wrote:
> Hi John,
>
>
> On 07/21/2015 09:02 PM, John Rose wrote:
>> Yes, that will work, and I think it is cleaner than what we had
>> before, as well as providing the new required functionality.
>>
>> Reviewed; please get a second reviewer.
>
> thank you for the review! I'll ask Vladimir K., maybe he has time to
> look at the newest webrev.
>
>>
>> — John
>>
>> P.S. If the unit tests want to test (via the whitebox API) whether an
>> intrinsic was compiled successfully, we might want to expose
>> Compile::gather_intrinsic_statistics, etc.  But not in this change set.
>
> That is an interesting idea. We'd also have to see if current tests
> require such functionality or if SQE plans to add tests requiring that
> functionality.
>
>>
>> P.P.S. As I think I said before, I wish we had a way to consolidate
>> the switch statements further (into vmSymbols.hpp).  But I don't see a
>> clean way to do it.
>
> Yes, that would be nice. I've not seen a good way to do that, partly
> because inconsistencies between the way C1 and C2 depends on the value
> of command-line flags.
>
> Thank you!
>
> Best regards,
>
>
> Zoltan
>
>>
>> On Jul 21, 2015, at 8:19 AM, Zoltán Majó <zoltan.majo at oracle.com
>> <mailto:zoltan.majo at oracle.com>> wrote:
>>>
>>> Here is the newest webrev:
>>> - top:http://cr.openjdk.java.net/~zmajo/8130832/top/
>>> <http://cr.openjdk.java.net/%7Ezmajo/8130832/top/>
>>> -
>>> hotspot:http://cr.openjdk.java.net/~zmajo/8130832/hotspot/webrev.03/
>>> <http://cr.openjdk.java.net/%7Ezmajo/8130832/hotspot/webrev.03/>
>>
>