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

[Bob Vandette]

David,

I assume that you are going to push this change independent of the ARM code removal JEP, right?

I just changed the JEP to “Propose to Target” and will merge any updates with my patch once
it is “Targeted”.


Bob.

> On Sep 23, 2018, at 12:26 PM, David Holmes <David.Holmes at oracle.com> 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