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

Thu Jun 11 21:24:27 UTC 2020

Hi Xin,

In general I think the patch looks good.

I am missing strict name checking. (I want to see an error on startup if 
the user has specified unknown intrinsic names.) I see that the lazy 
initialization of the intrinsic name tables might make it non-trivial to 
find a good place to do that. I am ok if you follow up on that in a 
future patch.

Best regards,
Nils Eliasson

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