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

[Nils Eliasson]

Hi Zoltan,

Some comments:

IntrinsicDisabledTest.java:

>> return  >> System.getProperty("java.vm.name").toLowerCase().contains("server");
>>

think the best practice is to use Platform.isServer() ("import 
jdk.test.lib.Platform;").

compilerDirectives.cpp:

I think the canonilization of the list belongs at the construction site, 
and not do at the (hot) use site. Preferably we would agree on using the 
',' separator in all use case (it only has internal uses). The 
compilecommand parser should be straightforward to fix. The VM flag may 
be parsed by a common parser that we can't change - then the vmflag 
value should be canonicalized during CompilerBroker_init or similar.

If there is some reason to why that doesn't work then I would suggest 
moving the canonicalization to DirectiveSet constructor and 
DirectiveSet::clone so it only happens once per DirectiveSet.

Best regards,
Nils





On 2015-10-23 09:51, 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. > > 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 >>> >


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20151023/3dc9440a/attachment.html>