[9] RFR(S): 8138651: -XX:DisableIntrinsic matches intrinsics overly eagerly

[Zoltán Majó]

Hi Vladimir,


thank you for the feedback!

On 10/07/2015 04:37 AM, Vladimir Kozlov wrote:
> To be precise DisableIntrinsic is ccstrlist option, not ccstr. Yes, 
> the actual type is the same.
>
> An other concern is separators since format could be different if 
> option specified in file. Look how we do search in DeoptimizeOnlyAt 
> string.

I've checked and DisableIntrinsic supports accumulation of argument 
values: If -XX:DisableIntrinsic is specified multiple times on the 
command line, all argument values are concatenated into one argument. In 
that case, '\n' is used as separator. I updated the webrev to support 
"\n" as a separator.

If DisableIntrinsic is used on the per-method level (i.e., with 
-XX:CompileCommand=option,...), HotSpot expects the type of 
DisableIntrinsic to be 'ccstr' and not 'ccstrlist'. That does not allow 
specifying a list of intrinsics to be disabled (e.g., 
_getInt,_getInVolatile,_hashCode) and is inconsistent with the 
declaration of DisableIntrinsic in globals.hpp.

To address this problem, the webrev changes the type of the per-method 
level DisableIntrinsic flag to 'ccstrlist'. For per-method ccstrlists, 
the separator is a whitespace (internally). I've updated the webrev to 
support whitespace as a separator as well.

I noticed an other problem while working on this fix: If 
DisableIntrinsic is specified multiple times for the same method, 
argument values do not accumulate. For example, with

-XX:CompileCommand=option,sun.misc.Unsafe::putChar,ccstrlist,DisableIntrinsic,_getInt,_getIntVolatile
-XX:CompileCommand=option,sun.misc.Unsafe::putChar,ccstrlist,DisableIntrinsic,_hashCode

only '_hashCode' will be disabled for 'putChar'. That is also 
inconsistent with the way DisableIntrinsic works when used globally 
(with -XX:DisableIntrinsic).

This inconsistency should be addressed, but as the fix requires 
significant changes to CompilerOracle, I would like to take care of that 
separately. I've filed an RFE for that:

https://bugs.openjdk.java.net/browse/JDK-8140322

I hope that is fine.

Here is the updated webrev:
http://cr.openjdk.java.net/~zmajo/8138651/webrev.01/

Testing:
- JPRT (testset hotspot, including the newly added 
IntrinsicDisabledTest.java test).

Thank you and best regards,


Zoltan

>
> Thanks,
> Vladimir
>
> On 10/6/15 8:00 PM, Zoltán Majó wrote:
>> Hi,
>>
>>
>> please review the patch for JDK-8138651.
>>
>> https://bugs.openjdk.java.net/browse/JDK-8138651
>>
>> Problem: The DisableIntrinsic flag does not disable intrinsics
>> accurately. For example, -XX:DisableIntrinsic=_copyOfRange disables both
>> the intrinsic with the ID _copyOfRange and the intrinsic with the 
>> _copyOf.
>>
>> Solution: Change the processing of the DisableIntrinsic flag (both
>> globally and on a per-method level).
>>
>> Webrev: http://cr.openjdk.java.net/~zmajo/8138651/webrev.00/
>>
>> Testing:
>> - JPRT (testset hotspot);
>> - executed the the newly added test
>> compiler/intrinsics/IntrinsicDisabledTest.java with/without the fix on
>> all platforms, the test behaves as expected.
>>
>> Thank you and best regards,
>>
>>
>> Zoltan
>>