RFR (XL) 8031320: Use Intel RTM instructions for locks

[David Holmes]

Hi Vladimir,

The original RFR states:

All new RTM flags are declared as experimental and require to specify 
"UnlockExperimentalVMOptions" flag.

but some of the flags are simply product. ??

I also see a lot of code is conditional on INCLUDE_RTM_OPT but I don't 
see a makefile change that enables this:

http://cr.openjdk.java.net/~kvn/8031320_9/webrev.01/

??

Thanks,
David


On 21/03/2014 10:46 AM, Vladimir Kozlov wrote:
> Thank you, Dan
>
> I fixed what you suggested and answered your questions. I will push
> these changes into our jdk9 hs-comp repo since other reviewers said okay.
>
> Thanks,
> Vladimir
>
> On 3/20/14 3:49 PM, Daniel D. Daugherty wrote:
>> On 3/19/14 11:26 PM, Vladimir Kozlov wrote:
>>> Thank you, Dan
>>>
>>> I updated webrev.01 with your and last Igor's suggestions. See my
>>> other mail with description of changes in it (mostly refactoring in
>>> macroAssembler_x86.cpp).
>>>
>>> http://cr.openjdk.java.net/~kvn/8031320_9/webrev.01/
>>>
>>
>> src/share/vm/runtime/rtmLocking.hpp
>>      line 88: static uintx* rtm_calculation_flag_adr() { return
>> &_calculation_flag; }
>>          Typo: 'rtm_calculation_flag_adr' -> 'rtm_calculation_flag_addr'
>
> Fixed
>
>>
>> src/cpu/x86/vm/assembler_x86.cpp
>>      line 3040: void Assembler::xend() {
>>          nit: Looks like these functions are in alpha order so xend()
>>          should be before xgetbv(). Sorry I missed this in the previous
>>          round.
>
> Fixed.
>
>> src/share/vm/opto/graphKit.cpp
>>      line 3154: if (UseBiasedLocking &&
>> PrintPreciseBiasedLockingStatistics) {
>>          This looks like another more general fix that should be
>>          backported.
>
> Agree.
>
>>
>>      line 3160: flock->create_rtm_lock_counter(sync_jvms()); //
>> sync_jvms used to get current bci
>>          Why are the counters always created?
>
> I moved checks when to create counters inside create_rtm_lock_counter()
> method because I did not want to add #if into graphkit.cpp. Counters
> will be created when:
> (C->profile_rtm() || (PrintPreciseRTMLockingStatistics && C->use_rtm())
>
>> src/share/vm/opto/macro.hpp
>>      The placement of the new _has_locks field seems strange.
>>      Other fields are above the functions.
>
> You are right. I moved it after ProjNode *_resproj; field.
>
>>
>> src/share/vm/opto/runtime.hpp
>>      line 90: assert(_next == NULL || next == NULL, "already set");
>>          This looks like another more general fix that should be
>>          backported.
>
> I will collect all your notes what we should backport into 7u.
>
>>
>> src/share/vm/opto/runtime.cpp
>>      line 1360   } else if (tag == NamedCounter::RTMLockingCounter) {
>>      line 1361     c = new
>> RTMLockingNamedCounter(strdup(st.as_string()));
>>          Previous new code is in #if INCLUDE_RTM_OPT ... #endif
>>          Is there some reason for the difference?
>
> RTMLockingCounters* is defined in rtmLocking.hpp and its methods which
> are used in guarded code are defined only in rtmLocking.cpp on x86.
>
> The enum NamedCounter::RTMLockingCounter and RTMLockingNamedCounter are
> defined on all platforms in runtime.hpp.
>
>>
>>      line 1370:  c->set_next(NULL);
>>          This looks like another more general fix that should be
>>          backported.
>
> Yes, it is old bug which I fixed and needs to be backported.
>
>>
>> src/share/vm/opto/type.cpp
>>      line 4383: return make(ptr, _metadata, offset);
>>          This does not seem to be RTM related and I don't see that
>>          file modified in the original webrev. Is this an unrelated
>>          change that managed to sneak in?
>
> It is related. I said about it in one of previous mails:
>
> "In phase1.cpp I changed to correct TypeMetadataPtr for MDO pointer
> instead of original RawPtr. And I hit bug in TypeMetadataPtr::xmeet()
> and fixed it."
>
> It is general bug but it affects only 9 and 8u with removed PermGen. It
> will be fixed with backport of these changes.
>
> Thanks,
> Vladimir
>
>>
>> Dan
>>
>>