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

David Holmes david.holmes at oracle.com
Wed Sep 26 13:49:53 UTC 2018 

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