RFR: 8248817: Windows: Improving common cross-platform code

[David Holmes]

Latest changes look good to me!

Thanks,
David

On 17/07/2020 8:00 am, Ludovic Henry wrote:
> I've upload these latest changes to http://cr.openjdk.java.net/~burban/luhenry/8248817-atomics/webrev.04
> 
> ________________________________________
> From: Ludovic Henry <luhenry at microsoft.com>
> Sent: Thursday, July 16, 2020 11:27
> To: Kim Barrett
> Cc: David Holmes; hotspot-runtime-dev at openjdk.java.net; openjdk-aarch64
> Subject: Re: RFR: 8248817: Windows: Improving common cross-platform code
> 
> Hi Kim,
> 
>> atomic_windows_x86.hpp:
>> I think retaining the "stub" nomenclature is misleading; "stub" has a
>> somewhat specific meaning in low-level HotSpot code.  "intrinsic"
>> might be a better choice.
> 
> Ok, let me rename that to IntrinsicName and IntrinsicType.
> 
>> atomic_windows_x86.hpp:
>> I'm guessing that as a followup, as part of the aarch64 port, these
>> will probably be changed to dispatch on the memory order to choose the
>> appropriately ordered intrinsics.  But what's being proposed seems
>> sufficient for now.
> 
> This code will not be shared directly with aarch64, even though it is very similar. As part of landing the Windows-AArch64 changes, we can imagine merging the two into src/hotspot/os/windows/atomic_windows.hpp for example.
> 
>> atomic_windows_x86.hpp:
>> I noticed that HotSpot's cmpxchg argument order is different than the
>> MSVC intrinsic's argument order.  That might be worthy of a comment.
> 
> Good point, let me add a comment.
> 
>> That parameter alignment problem doesn’t seem to have been fixed.
> 
> Bernhard (whom I'm using his CR) hasn't had a chance to upload it yet, I'll let you know once it's done.
> 
>> Nice to see the stub functions go away.
> 
> It does indeed seems to simplify the code drastically, so happy to participate to that as well :)
> 
> Thank you,
> 
> ________________________________________
> From: Kim Barrett <kim.barrett at oracle.com>
> Sent: Thursday, July 16, 2020 11:20
> To: Ludovic Henry
> Cc: David Holmes; hotspot-runtime-dev at openjdk.java.net; openjdk-aarch64
> Subject: Re: RFR: 8248817: Windows: Improving common cross-platform code
> 
>> On Jul 16, 2020, at 11:39 AM, Ludovic Henry <luhenry at microsoft.com> wrote:
>>
>> Hi David,
>>
>>> I'm not very good with templates so I've asked Kim Barrett if he can
>>> take a look at this aspect.
>>
>> Sounds good, looking forward for his review.
> 
> atomic_windows_x86.hpp:
> I think retaining the "stub" nomenclature is misleading; "stub" has a
> somewhat specific meaning in low-level HotSpot code.  "intrinsic"
> might be a better choice.
> 
> atomic_windows_x86.hpp:
> I'm guessing that as a followup, as part of the aarch64 port, these
> will probably be changed to dispatch on the memory order to choose the
> appropriately ordered intrinsics.  But what's being proposed seems
> sufficient for now.
> 
> atomic_windows_x86.hpp:
> I noticed that HotSpot's cmpxchg argument order is different than the
> MSVC intrinsic's argument order.  That might be worthy of a comment.
> 
>> I'm updating the webrev at https://nam06.safelinks.protection.outlook.com/?url=http:%2F%2Fcr.openjdk.java.net%2F~burban%2Fluhenry%2F8248817-atomics%2Fwebrev.03&data=02%7C01%7Cluhenry%40microsoft.com%7C06667a1f27ef4b6bc47208d829b5d4c7%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637305208221370933&sdata=c278Ey9Aoc%2BOARBRc1SRxIgdBt9N5d3rf9jlsgqFamg%3D&reserved=0 with the proper realignement of the parameters.
> 
> That parameter alignment problem doesn’t seem to have been fixed.
> 
> Other than these minor nits, this change looks good to me.
> 
> Nice to see the stub functions go away.
>