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

David Holmes david.holmes at oracle.com
Wed Oct 3 07:54:22 UTC 2018 

These changes have now been pushed.

David

On 27/09/2018 11:01 PM, David Holmes wrote:
> 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