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

[David Holmes]

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
>