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

[Vladimir Kozlov]

On 3/20/14 6:03 PM, David Holmes wrote:
> 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. ??

After I sent original webrev with all flags experimental Staffan Friberg 
(from performance group) asked to convert main flags into product. 
Otherwise we can't publish any performance results if we decide to do that.

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

It is defined in globalDefinitions.hpp:

+ #if defined(X86) && defined(COMPILER2) && !defined(JAVASE_EMBEDDED)
+ // Include Restricted Transactional Memory lock eliding optimization
+ #define INCLUDE_RTM_OPT 1

Thanks,
Vladimir

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