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

[Liu, Xin]

Hi, 

Please allow me to ping for this code review.

Here is the latest webrev: 
http://cr.openjdk.java.net/~xliu/8151779/05/webrev/
Incremental diff: http://cr.openjdk.java.net/~xliu/8151779/r4_to_r5.diff

I verified it in submit repo a week ago. I also double-check the patch still can patch to TIP and pass both hotspot:tier1 and gtest:all.  

Here is log message I got from mach-5. 
Job: mach5-one-phh-JDK-8151779-1-20200513-1821-11015755

BuildId: 2020-05-13-1820211.hohensee.source

No failed tests

Tasks Summary

EXECUTED_WITH_FAILURE: 0
NOTHING_TO_RUN: 0
KILLED: 0
HARNESS_ERROR: 0
FAILED: 0
PASSED: 101
UNABLE_TO_RUN: 0
NA: 0


Thanks, 
--lx



On 5/13/20, 12:03 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, Vladimir, 

    > 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.
    ----
    If -XX:ControlIntrinsic=+_updateBytesCRC32 can't win over -XX:-UseCRC32Intrinsics,  it will come back the justification of JBS-8151779: 
    Why do we need to support the usage -XX:ControlIntrinsic=+_updateBytesCRC32? If a user doesn't set +updateBytesCRC32, it's still enabled. 

    I read the description of "JBS-8235981" and "JBS-8151779" again. I try to understand in this way. The option 'UseCRC32Intrinsics' is the consolidation of 3 real intrinsics [_updateCRC32, _updateBytesCRC32, _updateByteBufferCRC32]. It represents some sorta hardware capabilities to make those intrinsics optimal. If UseCRC32Intrinsics is OFF,  it will not make sense to intrinsify them anymore because inliner can deliver the similar result.  

    Quote from JBS-8235981 "Right now, there's no way to introduce experimental intrinsics which are turned off by default and let users enable them on their side. "
    Currently, once a user declares one new intrinsics in VM_INTRINSICS_DO,  it's enabled. It might not be true in the future. 
    i.e. A develop can declare an intrinsic but mark it turn-off by default. He will try it out by -XX:ControlIntrinsic=+_myNewIntrinsic in his development stage. 

    Do I catch up your intention this time? if yes, could you take a look at this new revision? I think I meet the requirement.   
    Webrev: http://cr.openjdk.java.net/~xliu/8151779/05/webrev/
    Incremental diff: http://cr.openjdk.java.net/~xliu/8151779/r4_to_r5.diff

    Here is the change log from rev04.
    1) An intrinsic is enabled if and only if neither ControlIntrinsic nor the corresponding UseXXXIntrinsics disables it.
    The implementation is still in vmIntrinsics::is_disabled_by_flags(vmIntrinsics::ID id).

    2) I introduce a compact data structure TriBoolArray. It compresses an array of Tribool.  Each tribool only takes 2 bits now.
    I also took Coleen's suggestion to put TriBool and TriBoolArray in a standalone file "utilities/tribool.hpp". A new gtest is attached. 

    3) Correct some typos. Thank you David pointed them out. 

    Thanks,
    --lx


    On 5/12/20, 12:59 AM, "David Holmes" <david.holmes 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,

        Sorry for the delay in getting back to this.

        On 5/05/2020 7:37 pm, Liu, Xin wrote:
        > Hello, David and Nils
        >
        > Thank you to review the patch.  I went to brush up my English grammar and then update my patch to rev04.
        > https://cr.openjdk.java.net/~xliu/8151779/04/webrev/
        > Here is the incremental diff:  https://cr.openjdk.java.net/~xliu/8151779/r3_to_r4.diff  It reflect changes based on David's feedbacks. I really appreciate that you review so carefully and found so many invaluable suggestions. TBH, I don't understand Amazon's copyright header neither. I choose the simple way to dodge that problem.

        In vmSymbols.hpp

        +  // 1. Disable/Control Intrinsic accept a list of intrinsic IDs.

        s/accept/accepts/

        +  //    their final value are subject to hardware inspection
        (VM_Version::initialize).

        s/value/values/

        Otherwise all my nits have been addressed - thanks.

        I don't need to see a further webrev.

        Thanks,
        David
        -----

        > Nils points out a very tricky question. Yes, I also notice that each TriBool takes 4 bytes on x86_64. It's a natural machine word and supposed to be the most efficient form.  As a result, the vector control_words take about 1.3Kb for all intrinsics. I thought it's not a big deal, but Nils brought up that each DirectiveSet will increase from 128b to 1440b.  Theoretically, the user may provide a CompileCommandFile which consists of hundreds of directives. Will hotspot have hundreds of DirectiveSet in that case?
        >
        > Actually, I do have a compacted container of TriBool. It's like a vector<bool> specialization.
        > https://cr.openjdk.java.net/~xliu/8151779/TriBool.cpp
        >
        > The reason I didn't include it because I still feel that a few KiloBytes memories are not a big deal.  Nowadays, hotspot allows Java programmers allocate over 100G heap.  Is it wise to increase software complexity to save KBs?
        >
        > If you think it matters, I can integrate it.  May I update TriBoolArray in a standalone JBS? I have made a lot of changes. I hope I can verify them using KitchenSink?
        >
        > For the second problem,  I think it's because I used 'memset' to initialize an array of objects in rev01.   Previously, I had code like this:
        > memset(&_intrinsic_control_words[0], 0, sizeof(_intrinsic_control_words));
        >
        > This kind of usage will be warned as -Werror=class-memaccess in g++-8.  I have fixed it since rev02. I use DirectiveSet::fill_in(). Please check out.
        >
        > Thanks,
        > --lx
        >