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

[Vladimir Kozlov]

On 10/23/15 3:51 PM, Zoltán Majó wrote:
> 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.

Good.

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

Yes.

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

Yes, it is fine to fix separately.

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

Will you update later (in next rfe) next method to print all items on 
list 'v'? Should it be 'ccstrlist v'?:

+   void print_ccstrlist(outputStream* st, ccstr n, ccstr v, bool mod) { 
print_ccstr(st, n, v, mod); }

Otherwise changes looks good.

Thanks,
Vladimir

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