RFR[M]: 8151779: Some intrinsic flags could be replaced with one general flag
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Mon May 4 15:22:45 UTC 2020
On 5/3/20 10:08 PM, David Holmes wrote:
> Hi Vladimir, Xin,
>
> Overall this seems fine to me. A few style nits below.
>
> On 2/05/2020 11:00 am, Vladimir Kozlov wrote:
>> Hi
>>
>> I am CCing to runtime group too. I would like to see comments about
>> these changes. No need to look on compiler's changes.
>>
>> The latest https://cr.openjdk.java.net/~xliu/8151779/02/webrev/
>
> src/hotspot/share/classfile/vmSymbols.cpp
>
> ! assert(!strcmp(nt[vmIntrinsics::_hashCode], "_hashCode"), "lined
> up");
>
> Avoid implicit bools - compare against 0
>
> ! init_vm_intrnsic_name_table();
>
> Typo: intrnsic -> intrinsic (multiple places)
>
> + if (!strcmp(name, nt[index])) return ID_from(index);
>
> Avoid implicit bools - compare against 0
> "return" on newline
> Use { }
>
> + * There're 2 approaches to control intrinsics.
>
> "there're" -> "there are"
>
> s/control/controlling/
>
> + * 1.Disable/ControlIntrinsic
>
> space after .
>
> + * ControlIntrinsic is recommented.
>
> Typo: recommented -> recommended
>
> + * Currently, DisableIntrinsic list
>
> insert: ^the^ DisableIntrinsic list
>
> + * 2.some UseXXXIntrinsics options. eg. UseAESIntrinsics
>
> space after .
>
> Suggestion: s/some/Explicit/
>
> + * Each option can control a set of intrinsics. User can specify
> them but
>
> s/User/The user/
>
> + * they are subjected to hardward inspection(VM_Version::initialize).
>
> s/subjected/subject/
> s/hardward/hard-wired/
> space before (
>
> + for (ControlIntrinsicIter iter(ControlIntrinsic); *iter; ++iter) {
> + for (ControlIntrinsicIter iter(DisableIntrinsic,
> true/*disable_all*/); *iter; ++iter) {
>
> Avoid implicit bools
> Space before /*
>
> + if (b.is_default())
> + return !vmIntrinsics::is_disabled_by_flags(id);
> + else
> + return b;
>
> Use { }
>
> --
>
> src/hotspot/share/classfile/vmSymbols.hpp
>
> tribool -> TriBool
>
> Per hotspot style guide (yes there are existing types that don't
> follow tis).
https://cr.openjdk.java.net/~xliu/8151779/02/webrev/src/hotspot/share/classfile/vmSymbols.hpp.udiff.html
I was going to make this same comment about capitalization. This tribool
is an odd class. It seems almost generally useful, like for command line
flags so being in vmSymbols doesn't seem like the right place, but it
can stay here unless we catch some other code change trying to do the
same thing somewhere else.
This change makes me really want someone to do this RFE:
https://bugs.openjdk.java.net/browse/JDK-8243066
https://cr.openjdk.java.net/~xliu/8151779/02/webrev/src/hotspot/share/compiler/compilerDirectives.hpp.udiff.html
This could probably use a forward declaration of TriBool and include
vmSymbols.hpp in the cpp file.
Otherwise seems ok. I didn't review the parsing code.
Coleen
>
> ---
>
> test/hotspot/gtest/classfile/test_vmSymbols.cpp
>
> Apparently Amazon don't use years in their copyright notices and this
> is being changed elsewhere.
>
> Thanks,
> David
>
>> Good work.
>>
>> On 4/24/20 1:33 AM, Liu, Xin wrote:
>>> Hi,
>>>
>>> May I get reviewed for this new revision?
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8151779
>>> webrev: https://cr.openjdk.java.net/~xliu/8151779/01/webrev/
>>>
>>> I introduce a new option -XX:ControlIntrinsic=+_id1,-id2...
>>> The id is vmIntrinsics::ID. As prior discussion, ControlIntrinsic
>>> is expected to replace DisableIntrinsic.
>>> I keep DisableIntrinsic in this revision. DisableIntrinsic prevails
>>> when an intrinsic appears on both lists.
>>
>> Yes, you have to keep DisableIntrinsic for now. We will deprecate it
>> later.
>>
>>>
>>> I use an array of tribool to mark each intrinsic is enabled or not.
>>> In this way, hotspot can avoid expensive string querying among
>>> intrinsics.
>>> A Tribool value has 3 states: Default, true, or false.
>>> If developers don't explicitly set an intrinsic, it will be
>>> available unless is disabled by the corresponding UseXXXIntrinsics.
>>> Traditional Boolean value can't express fine/coarse-grained control.
>>> Ie. We only go through those auxiliary options UseXXXIntrinsics if
>>> developers don't control a specific intrinsic.
>>>
>>> I also add the support of ControlIntrinsic to CompilerDirectives.
>>>
>>> Test:
>>> I reuse jtreg tests of DisableIntrinsic. Add add more @run
>>> annotations to verify ControlIntrinsics.
>>> I passed hotspot:Tier1 test and all tests on x86_64/linux.
>>
>> Good. I submitted hotspot tier1-3 testing.
>>
>> Thanks,
>> Vladimir
>>
>>>
>>> Thanks,
>>> --lx
>>>
>>> On 4/17/20, 7:22 PM, "hotspot-compiler-dev on behalf of Liu, Xin"
>>> <hotspot-compiler-dev-bounces at openjdk.java.net on behalf of
>>> xxinliu at amazon.com> wrote:
>>>
>>> Hi, Vladimir,
>>>
>>> Thanks for the clarification.
>>> Oh, yes, it's theoretically possible, but it's tedious. I am
>>> wrong at that point.
>>> I think I got your point. ControlIntrinsics will make
>>> developer's life easier. I will implement it.
>>>
>>> Thanks,
>>> --lx
>>>
>>>
>>> On 4/17/20, 6:46 PM, "Vladimir Kozlov"
>>> <vladimir.kozlov at oracle.com> wrote:
>>>
>>> CAUTION: This email originated from outside of the
>>> organization. Do not click links or open attachments unless you can
>>> confirm the sender and know the content is safe.
>>>
>>>
>>>
>>> I withdraw my suggestion about EnableIntrinsic from
>>> JDK-8151779 because ControlIntrinsics will provide such
>>> functionality and will replace existing DisableIntrinsic.
>>>
>>> Note, we can start deprecating Use*Intrinsic flags (and
>>> DisableIntrinsic) later in other changes. You don't need to do
>>> everything at once. What we need now a mechanism to replace
>>> them.
>>>
>>> On 4/16/20 11:58 PM, Liu, Xin wrote:
>>> > Hi, Corey and Vladimir,
>>> >
>>> > I recently go through vmSymbols.hpp/cpp. I think I
>>> understand your comments.
>>> > Each UseXXXIntrinsics does control a bunch of intrinsics
>>> (plural). Thanks for the hint.
>>> >
>>> > Even though I feel I know intrinsics mechanism of hotspot
>>> better, I still need a clarification of JDK- 8151779.
>>> >
>>> > There're 321 intrinsics
>>> (https://chriswhocodes.com/hotspot_intrinsics_jdk15.html).
>>> > If there's no any option, they are all available for
>>> compilers. That makes sense because intrinsics are always beneficial.
>>> > But there're reasons we need to disable a subset of them.
>>> A specific architecture may miss efficient instructions or fixed
>>> functions. Or simply because an intrinsic is buggy.
>>> >
>>> > Currently, JDK provides developers 2 ways to control
>>> intrinsics. > 1. Some diagnostic options. Eg. InlineMathNatives,
>>> UseBase64Intrinsics.
>>> > Developers can use one option to disable a group of
>>> intrinsics. That is to say, it's a coarse-grained approach.
>>> >
>>> > 2. DisableIntrinsic="a,b,c"
>>> > By passing a string list of vmIntrinsics::IDs, it's
>>> capable of disabling any specified intrinsic.
>>> >
>>> > But even putting above 2 approaches together, we still
>>> can't precisely control any intrinsic.
>>>
>>> Yes, you are right. We seems are trying to put these 2
>>> different ways into one flag which may be mistake.
>>>
>>> -XX:ControlIntrinsic=-_updateBytesCRC32C,-_updateDirectByteBufferCRC32C
>>> is a similar to -XX:-UseCRC32CIntrinsics but it
>>> requires more detailed knowledge about intrinsics ids.
>>>
>>> May be we can have 2nd flag, as you suggested
>>> -XX:UseIntrinsics=-AESCTR,+CRC32C, for such cases.
>>>
>>> > If we want to enable an intrinsic which is under control
>>> of InlineMathNatives but keep others disable, it's impossible now.
>>> [please correct if I am wrong here].
>>>
>>> You can disable all other from 321 intrinsics with
>>> DisableIntrinsic flag which is very tedious I agree.
>>>
>>> > I think that the motivation JDK-8151779 tried to solve.
>>>
>>> The idea is that instead of flags we use to control
>>> particular intrinsics depending on CPU we will use vmIntrinsics::IDs
>>> or other tables as you showed in your changes. It will
>>> require changes in vm_version_<cpu> codes.
>>>
>>> >
>>> > If we provide a new option EnableIntrinsic and put it
>>> least priority, then we can precisely control any intrinsic.
>>> > Quote Vladimir Kozlov "DisableIntrinsic list prevails if
>>> an intrinsic is specified on both EnableIntrinsic and
>>> DisableIntrinsic."
>>> >
>>> > "-XX:ControlIntrinsic=+_dabs,-_fabs,-_getClass" looks
>>> more elegant, but it will confuse developers with DisableIntrinsic.
>>> > If we decide to deprecate DisableIntrinsic, I think
>>> ControlIntrinsic may be a better option. Now I prefer to provide
>>> EnableIntrinsic for simplicity and symmetry.
>>>
>>> I prefer to have one ControlIntrinsic flag and deprecate
>>> DisableIntrinsic. I don't think it is confusing.
>>>
>>> Thanks,
>>> Vladimir
>>>
>>> > What do you think?
>>> >
>>> > Thanks,
>>> > --lx
>>> >
>>> >
>>> > On 4/13/20, 1:47 PM, "hotspot-compiler-dev on behalf of
>>> Corey Ashford" <hotspot-compiler-dev-bounces at openjdk.java.net on
>>> behalf of cjashfor at linux.ibm.com> wrote:
>>> >
>>> > CAUTION: This email originated from outside of the
>>> organization. Do not click links or open attachments unless you can
>>> confirm the sender and know the content is safe.
>>> >
>>> >
>>> >
>>> > On 4/13/20 10:33 AM, Liu, Xin wrote:
>>> > > Hi, compiler developers,
>>> > > I attempt to refactor UseXXXIntrinsics for
>>> JDK-8151779. I think we still need to keep UseXXXIntrinsics options
>>> because many applications may be using them.
>>> > >
>>> > > My change provide 2 new features:
>>> > > 1) a shorthand to enable/disable intrinsics.
>>> > > A comma-separated string. Each one is an
>>> intrinsic. An optional tailing symbol + or '-' denotes enabling or
>>> disabling.
>>> > > If the tailing symbol is missing, it means enable.
>>> > > Eg.
>>> -XX:UseIntrinsics="AESCTR-,CRC32C+,CRC32-,MathExact"
>>> > > This jvm option will expand to multiple options
>>> -XX:-UseAESCTRIntrinsics, -XX:+UseCRC32CIntrinsics,
>>> -XX:-UseCRC32Intrinsics, -XX:UseMathExactIntrinsics
>>> > >
>>> > > 2) provide a set of macro to declare intrinsic
>>> options
>>> > > Developers declare once in intrinsics.hpp and
>>> macros will take care all other places.
>>> > > Here are example:
>>> https://cr.openjdk.java.net/~xliu/8151779/00/webrev/src/hotspot/share/compiler/intrinsics.hpp.html
>>>
>>> > > Ion Lam is overhauling jvm options. I am thinking
>>> how to be consistent with his proposal.
>>> > >
>>> >
>>> > Great idea, though to be consistent with the
>>> original syntax, I think
>>> > the +/- should be in front of the name:
>>> >
>>> > -XX:UseIntrinsics=-AESCTR,+CRC32C,...
>>> >
>>> >
>>> > > I handle UseIntrinsics before
>>> VM_Version::initialize. It means that platform-specific
>>> initialization still has a chance to correct those options.
>>> > > If we do that after VM_Version::initialize, some
>>> intrinsics may cause JVM crash. Eg. +UseBase64Intrinsics on x86_64
>>> Linux.
>>> > > Even though this behavior is same as
>>> -XX:+UseXXXIntrinsics, from user's perspective, it's not
>>> straightforward when JVM overrides what users specify implicitly.
>>> It's dilemma here, stable jvm or fidelity of cmdline. What do you
>>> think?
>>> > >
>>> > > Another problem is naming convention. Almost all
>>> intrinsics options use UseXXXIntrinsics. One exception is
>>> UseVectorizedMismatchIntrinsic.
>>> > > Personally, I think it should be "UseXXXIntrinsic"
>>> because one option is for one intrinsic, right? Is it possible to
>>> change this name convention?
>>> >
>>> > Some (many?) intrinsic options turn on more than one
>>> .ad instruct
>>> > instrinsic, or library instrinsics at the same
>>> time. I think that's why
>>> > the plural is there. Also, consistently adding the
>>> plural allows you to
>>> > add more capabilities to a flag that initially only
>>> had one intrinsic
>>> > without changing the plurality (and thus backward
>>> compatibility).
>>> >
>>> > Regards,
>>> >
>>> > - Corey
>>> >
>>> >
>>>
>>>
More information about the hotspot-runtime-dev
mailing list