RFR[M]: 8151779: Some intrinsic flags could be replaced with one general flag
Vladimir Kozlov
vladimir.kozlov at oracle.com
Thu May 7 20:29:12 UTC 2020
On 5/3/20 3:48 PM, Liu, Xin wrote:
> Hi, Vladimir,
>
> Thank you to review the patch!
>
> For the failure, it's actually a bug. _updateCRC32 should be enabled no matter how on 32bit x86. Here is the new revision with bugfix.
> http://cr.openjdk.java.net/~xliu/8151779/03/webrev/
> I made an incremental diff between rev02 and rev03: http://cr.openjdk.java.net/~xliu/8151779/r2_to_r3.diff
>
> 1. The bug was because abstractCompiler miss out to check vm_intrinsic_control_words[].
> Previously, Class vmIntrinsics provided multiple interfaces for intrinsic availability. (https://hg.openjdk.java.net/jdk/jdk/file/4198213fc371/src/hotspot/share/classfile/vmSymbols.hpp#l1696)
>
> abstractCompiler.cpp and library_call.cpp is_disabled_by_flags () but stubGenerator_x86_64.cpp uses is_intrinsic_available().
> I promote "is_disabled_by_flags()" to the core interface, leave more comments on it, and keep is_intrinsic_available() for compatibility.
okay
>
> 2. add +/- UseCRC32Intrinsics to IntrinsicAvailableTest.java
> The purpose of that test is not to generate a CRC32 intrinsic. Its purpose is to check if compilers determine to intrinsify _updateCRC32 or not.
> Mathematically, "UseCRC32Intrinsics" is a set = [_updateCRC32, _updateBytesCRC32, _updateByteBufferCRC32].
> "-XX:-UseCRC32Intrinsics" disables all 3 of them. If users use -XX:ControlIntrinsic=+_updateCRC32 and -XX:-UseCRC32Intrinsics, _updateCRC32 should be enabled individually.
No, I think we should preserve current behavior when UseCRC32Intrinsics is off then all corresponding intrinsics are
also should be off. This is the purpose of such flags - to be able control several intrinsics with one flag.
Otherwise you have to check each individual intrinsic if CPU does not support them. Even if code for some of these
intrinsics can be generated on this CPU. We should be consistent, otherwise code can become very complex to support.
>
> Yes, it will crash if compilers do generate _updateCRC32 without UseCRC32Intrinsics. It's because hotspot doesn't generate corresponding stubs, which are controlled by UseCRC32Intrinsics.
We should not allow crashes in all cases. JVM should exit gracefully with error message.
>
> That's by design. hotspot has made huge efforts to enable as many intrinsics as it can. If a user explicitly enables an unsupported intrinsics, he/she must do it for reasons. One possible scenario is that he is developing a new intrinsic.
No, people make mistakes. We should notify them that they made mistake. These flags rules are for Java users who most
likely don't know how correctly control JVM features.
And JVM developers are smart enough to bypass all restrictions we put into VM. We don't need to relax VM checks for them.
>
> Actually, the reason c1/c2 crash because both of them choose hard-landing. Eg. LibraryCallKit::inline_updateCRC32() assumes it has the stub.
> I don't know why, but templateIntercept seems to choose to tolerate it.
Interpreter checks UseCRC32Intrinsics flag for code generation. C2 and C1 do the same in
AbstractCompiler::is_intrinsic_available().
>
> On legacy hosts without CLMUL, -XX:+UseCRC32Intrinsics will be drop. It's still safe to run because IntrinsicAvailableTest.java doesn't attempt to compile the intrinsic method.
The test can check for negative results too.
>
> 3. I found an interesting optimization.
> We can use vm_intrinsic_control_words[] as a cache. I assume that no one changes those UseXXXIntrinsics options at the runtime.
> It can skip the mega-switch of vmIntrinsics::disabled_by_jvm_flags(), which might not be a big deal for optimizing compilers, but it can guarantee O(1) complexity for all toolchains.
Yes, after VM_Version_init() call intrinsics flags should not change.
Thanks,
Vladimir
>
> Thanks,
> --lx
>
> On 5/1/20, 8:36 PM, "hotspot-compiler-dev on behalf of Vladimir Kozlov" <hotspot-compiler-dev-bounces at openjdk.java.net on behalf of 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.
>
>
>
> Hi Xin
>
> compiler/intrinsics/IntrinsicAvailableTest.java test failed on old x86 machine which does not have CLMUL and as result
> can't use CRC32 intrinsic (UseCRC32Intrinsics flag is false). With -XX:ControlIntrinsic=+_updateCRC32 test failed with:
>
> java.lang.RuntimeException: Unexpected result: intrinsic for java.util.zip.CRC32.update() is enabled but intrinsic is
> not available at compilation level 1
> at compiler.intrinsics.IntrinsicAvailableTest.checkIntrinsicForCompilationLevel(IntrinsicAvailableTest.java:128)
> at compiler.intrinsics.IntrinsicAvailableTest.test(IntrinsicAvailableTest.java:138)
> at compiler.intrinsics.IntrinsicAvailableTest.main(IntrinsicAvailableTest.java:150)
>
> Regards,
> Vladimir
>
> On 5/1/20 6:00 PM, 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/
> >
> > 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