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

Sun Sep 23 22:26:32 UTC 2018

Hi David,

Okay, thanks. Note though that the exact code that you removed that was 
in the !is_MP() variant, is what we should really have all the time 
invariant of is_MP(). So in that new RFE, we need to basically throw 
away the is_MP() code and reintroduce the !is_MP() code.

Thanks,
/Erik

On 2018-09-24 00:18, David Holmes wrote:
> Hi Erik,
>
> I'll file a separate RFE for that.
>
> Thanks,
> David
>
> On 23/09/2018 4:19 PM, Erik Österlund wrote:
>> Hi David,
>>
>> Before I forget about it...
>> In src/hotspot/cpu/x86/jniFastGetField_x86_64.cpp
>>
>> Note that the is_MP() check was used to conditionally poke in an xor 
>> trick to create a data dependency to the safepoint counter. You 
>> removed the not is_MP() path with this patch. However, note that this 
>> use of forced data dependency to elide the need for an acquire or 
>> loadload fence is nonsense on x86 hardware, as loadload simply does 
>> not reorder. If you look at the corresponding SPARC code, you will 
>> find that there is no such confused xor trick, and rightfully so.
>>
>> So while said xor trick is simply not needed, it isn't incorrect to 
>> do it.
>>
>> I'll leave it up to you if you want to fix this in this change or 
>> not. But that xor trick is confused and should IMO be removed. It 
>> makes readers think it's there for a reason. But the assumption that 
>> it is needed when is_MP() is wrong.
>>
>> Thanks,
>> /Erik
>>
>> On 2018-09-23 18: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
>>