RFR[M]: 8151779: Some intrinsic flags could be replaced with one general flag

[Liu, Xin]

Hi, 

Ping for this code review. 

I've updated the rev02 a little bit.  Here is new revision. 
https://cr.openjdk.java.net/~xliu/8151779/02/webrev/

1. resolve merging conflict with TIP.
2. add fill_in functions to pass sanity test of submit repo. 
NOTHING_TO_RUN: 0
UNABLE_TO_RUN: 0
KILLED: 0
NA: 0
HARNESS_ERROR: 0
FAILED: 0
EXECUTED_WITH_FAILURE: 0
PASSED: 84

3. I also changed the description of ControlIntrinsic.  
java -XX:+PrintFlagsWithComments | grep ControlIntrinsic
ccstrlist ControlIntrinsic                         =                                        {diagnostic} {default}       Control intrinsics using a list of +/- (internal) names, separated by commas

thanks,
--lx


On 4/24/20, 1:40 AM, "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,  

    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.   

    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. 

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