RFR (M): 8188764: Obsolete AssumeMP and then remove all support for non-MP builds

David Holmes david.holmes at oracle.com
Thu Sep 27 13:01:43 UTC 2018 

Webrev was updated in place. Second occurrence was renamed.

Thanks,
David

On 26/09/2018 9:49 AM, David Holmes wrote:
> Thanks Boris, I'll fix the label problem.
> 
> I'm travelling at the moment so won't push till later next week. 
> Hopefully you'll have access to the hardware by then.
> 
> Thanks,
> David
> 
> On 26/09/2018 9:47 AM, Boris Ulasevich wrote:
>> Hi David,
>>
>>    For this change the most interesting would be to check it with 
>> ARMv5, but I do not have the hardware now. I hope I will get one ARMv5 
>> machine next week.
>>
>>    For ARMv7 the change is Ok. Almost Ok. There is a minor build issue:
>>
>> /home/boris/jdk-jdk/src/hotspot/cpu/arm/templateTable_arm.cpp:3724:9: 
>> error: redeclaration of ‘Label notVolatile’
>>     Label notVolatile;
>>           ^~~~~~~~~~~
>> /home/boris/jdk-jdk/src/hotspot/cpu/arm/templateTable_arm.cpp:3492:9: 
>> note: ‘Label notVolatile’ previously declared here
>>     Label notVolatile;
>>           ^~~~~~~~~~~
>> /home/boris/jdk-jdk/src/hotspot/cpu/arm/templateTable_arm.cpp: In 
>> static member function ‘static void 
>> TemplateTable::fast_storefield(TosState)’:
>> /home/boris/jdk-jdk/src/hotspot/cpu/arm/templateTable_arm.cpp:3885:9: 
>> error: redeclaration of ‘Label notVolatile’
>>     Label notVolatile;
>>           ^~~~~~~~~~~
>> /home/boris/jdk-jdk/src/hotspot/cpu/arm/templateTable_arm.cpp:3832:9: 
>> note: ‘Label notVolatile’ previously declared here
>>     Label notVolatile;
>>           ^~~~~~~~~~~
>>
>> regards,
>> Boris
>>
>> On 23.09.2018 19:26, David Holmes wrote:
>>> Hi Boris,
>>>
>>> Here is latest webrev with the ARM code updated as well.
>>>
>>> http://cr.openjdk.java.net/~dholmes/8188764/webrev/
>>>
>>> All is_MP removed except where needed for ARMv5 support, MP specific 
>>> instructions, and controlling of biased locking.
>>>
>>> I checked the DMB variants and we only generate for ARMv7 anyway, so 
>>> no issue there.
>>>
>>> Would very much appreciate whatever test builds you can do on 
>>> different ARM variants Boris!
>>>
>>> Thanks,
>>> David
>>>
>>> On 21/09/2018 9:41 AM, David Holmes wrote:
>>>> Hi Boris,
>>>>
>>>> On 21/09/2018 9:34 AM, Boris Ulasevich wrote:
>>>>> Hi David,
>>>>>
>>>>> On 20.09.2018 18:26, David Holmes wrote:
>>>>>> Hi Boris,
>>>>>>
>>>>>> Thanks for jumping on this. I hope you didn't spend too much time 
>>>>>> on it as I had actually started on the ARM code then decided I 
>>>>>> didn't need to make the changes, so I'd already gone through 
>>>>>> pretty much everything that is needed. My concern is with things 
>>>>>> like the change in
>>>>>>
>>>>>> src/hotspot/cpu/arm/assembler_arm_32.hpp
>>>>>>
>>>>>> where you removed the is_MP() check but in fact that needs to 
>>>>>> remain because the instruction pldw is not present on variants of 
>>>>>> the v7 architecture without the multi-processor extensions:
>>>>>
>>>>> Ok. Good point. Sure, my hasty proposal to get rid of is_MP() check 
>>>>> in arm32 codes was wrong.
>>>>>
>>>>> One note. I see there is an ambiguity here: is_MP() is a 
>>>>> multiprocessor system flag (returns true when _processor_count != 
>>>>> 1), but multi-processor extensions is an optional ARMv7 feature 
>>>>> which is not related with processors count, so pldw usage should 
>>>>> not be controlled by is_MP() flag.
>>>>
>>>> is_MP can't return true unless there is more than one processor 
>>>> (ignoring the pre-initialization case), so if there's more than one 
>>>> processor there must be a MP supprting architecture.
>>>>
>>>>>> http://www.keil.com/support/man/docs/armclang_asm/armclang_asm_pge1425899018492.htm 
>>>>>>
>>>>>> That makes me wonder whether any of the memory barrier 
>>>>>> instructions may also be missing in some v6/v7 variants as well? 
>>>>>
>>>>> Nobody complained :) And Reference Manual says DMB/DSB are not 
>>>>> optional.
>>>>
>>>> I need to double-check we don't define variants like "dmb sy" that 
>>>> are architecture specific.
>>>>
>>>>>> Plus I have to account for uniprocessor ARMv5 so need to see 
>>>>>> whether MacroAssembler::membar, for example, must also retain the 
>>>>>> is_MP() check.
>>>>>
>>>>> Do you want to delete this check? Processor count check seems quite 
>>>>> natural to see if it has sense to generate memory barriers.
>>>>
>>>> That is the general thrust of this change. We assume we always have 
>>>> MP and so always issue memory barriers.
>>>>
>>>>> By the way, MacroAssembler::fast_lock have 
>>>>> VM_Version::supports_ldrex() check assertion with message 
>>>>> "unsupported, yet?". Looks like it is not going to work correctly 
>>>>> on multiprocessor ARMv5.
>>>>
>>>> The ARM code does not support multiprocessor ARMv5.
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>>> Also in:
>>>>>>
>>>>>> src/hotspot/cpu/arm/stubGenerator_arm.cpp
>>>>>>
>>>>>> you can't remove the !is_MP() check and related code as that is 
>>>>>> needed for ARMv5 (uniprocessor) support.
>>>>>>
>>>>>> Similarly in
>>>>>>
>>>>>> src/hotspot/cpu/arm/vm_version_arm_32.cpp
>>>>>>
>>>>>> you'll probably still want to disable biased-locking for ARMv5.
>>>>>>
>>>>>> Cheers,
>>>>>> David
>>>>>>
>>>>>> On 20/09/2018 3:31 AM, Boris Ulasevich wrote:
>>>>>>> Hi David,
>>>>>>>
>>>>>>>  > I have not updated the about-to-be-removed Oracle ARM port code.
>>>>>>>
>>>>>>> Ok, but we can do the changes in ARM32 codes leaving ARM64 intact.
>>>>>>> I would propose the following change - I think we can either add 
>>>>>>> it to your update or to create a separate RFR after your patch 
>>>>>>> and JEP-340 commit:
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~bulasevich/8188764/webrev.00/
>>>>>>>
>>>>>>> regards,
>>>>>>> Boris
>>>>>>>
>>>>>>> On 18.09.2018 08:32, David Holmes wrote:
>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8188764
>>>>>>>> Webrev: http://cr.openjdk.java.net/~dholmes/8188764/webrev/
>>>>>>>>
>>>>>>>> As previously discussed in the RFC thread:
>>>>>>>>
>>>>>>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2018-September/034166.html 
>>>>>>>>
>>>>>>>>
>>>>>>>> this change obsoletes the AssumeMP flag and removes the bulk of 
>>>>>>>> the logic that is conditional on os::is_MP() as follows:
>>>>>>>>
>>>>>>>> 1. to gate the introduction of MP-specific features, notably 
>>>>>>>> memory barriers
>>>>>>>>
>>>>>>>> The is_MP check is removed and we will always issue memory barriers
>>>>>>>> or pre-pend lock prefix etc.
>>>>>>>>
>>>>>>>> 2. to align certain patchable code sequences (method entry, call 
>>>>>>>> sites)
>>>>>>>>
>>>>>>>> The is_MP is removed and we always align patchable locations.
>>>>>>>>
>>>>>>>> 3. to gate certain optimizations which are questionable on 
>>>>>>>> uniprocessors
>>>>>>>> (see quicken_jni_functions)
>>>>>>>>
>>>>>>>> These are psuedo-memory-barriers where we manufacture a 
>>>>>>>> data-dependency
>>>>>>>> instead of inserting mfence/lfence (x86 example). These are 
>>>>>>>> treated as
>>>>>>>> #1 and is_MP is removed.
>>>>>>>>
>>>>>>>> 4. to gate optimizations which are desirable on uniprocessors
>>>>>>>> (mutex/monitor short circuits)
>>>>>>>>
>>>>>>>> These are spin controls and so is_MP remains.
>>>>>>>>
>>>>>>>> I have not updated the about-to-be-removed Oracle ARM port code.
>>>>>>>>
>>>>>>>> Testing:
>>>>>>>>    - Tiers 1 -3 (mach5)
>>>>>>>>
>>>>>>>> Performance run TBD.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> David

More information about the hotspot-dev mailing list