RFR[M]: 8151779: Some intrinsic flags could be replaced with one general flag
Hohensee, Paul
hohensee at amazon.com
Tue Jun 23 17:35:12 UTC 2020
Looks good.
Thanks,
Paul
On 6/17/20, 1:04 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, Nils,
Thank you to review it.
I finally get gtest fixed on my side. I verify that webrev again. It
still can apply to TIP cleanly and pass both gtest:all and hotspot:tier1.
Call for another reviewer to approve it.
http://cr.openjdk.java.net/~xliu/8151779/05/webrev/
I think of your request and I've filed a follow-up issue: JDK-8247732. I
plan to use a constraint of globals.hpp and
install the constraint function to jvmFlagContraintsCompiler.hpp. for
directives, I will add validator in DirectiveSet::init_control_intrinsic.
thanks,
--lx
On 6/11/20 2:24 PM, Nils Eliasson 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,
>
> 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
>> >
>>
>>
>
More information about the hotspot-compiler-dev
mailing list