RFR[M]: 8151779: Some intrinsic flags could be replaced with one general flag
David Holmes
david.holmes at oracle.com
Mon May 4 02:08:13 UTC 2020
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).
---
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-compiler-dev
mailing list