RFR (M) 8186903: Remove j-types from Atomic

[David Holmes]

On 7/12/2017 2:00 PM, coleen.phillimore at oracle.com wrote:
> On 12/6/17 9:41 PM, David Holmes wrote:
>> Hi Coleen,
>>
>> On 7/12/2017 7:59 AM, coleen.phillimore at oracle.com wrote:
>>> Summary: Change parameter types from jlong, jint, jbyte to int64_t, 
>>> int32_t and int8_t respectively
>>
>> That's fine. But you also made a bunch of changes to rename the "ptr" 
>> variants to "long", and in the process changed intptr_t to int64_t. 
>> The change ptr->long isn't really accurate. And intptr_t and int64_t 
>> are different sizes on 32-bit! So this change seems problematic and 
>> out of scope by your own description. That said I thought we had/were 
>> getting rid of the "ptr" variants ??
> 
> Yes, I did rename atomic_add_ptr_entry to atomic_add_long_entry. This is 
> to match the other names which are currently empty, "long" and "byte". 
> This was a last vestige of the ptr variants.   It's only used on 64 bit 
> windows, so changing it to intptr_t to int64_t is correct.   For some 
> reason, there isn't a long variant on 32 bits.  Possibly because we 
> don't add jlong values in the vm?

Okay. Was hard to see this was all on 64-bit only.

Though not clear why this renaming needs to happen at all at this time? 
I'm finding it very confusing trying to see where we are going with this 
- shouldn't all the ptr variants become unused with the new Atomic API? 
I find them easier to spot when written as _ptr_ rather than _long_. 
(And on Windows a long isn't 64-bits anyway!)

> ent
> So this renaming makes it consistent with the other names, which then 
> can all be changed at once, when nice names are agreed upon.
>>
>>> Leave renaming functions for later change.
>>>
>>> Ran JPRT which builds more Oracle platforms, mach5 tier1-5 on 
>>> windows/linux x64 and tier1 on solaris-sparcv9.  Also built Zero 
>>> product mode (fails building fastdebug for some other reason).
>>>
>>> Other platforms: could you please test this patch?
>>>
>>> open webrev at http://cr.openjdk.java.net/~coleenp/8186903.01/webrev
>>> bug link https://bugs.openjdk.java.net/browse/JDK-8186903
>>>
>>> This change is for JDK 11.
>>
>> src/hotspot/cpu/x86/stubGenerator_x86_32.cpp
>>
>> Return type should be int32_t not int
>>
>> src/hotspot/os_cpu/windows_x86/os_windows_x86.hpp
>>
>> -  static intptr_t  (*atomic_xchg_long_func)     (jlong, volatile 
>> jlong*);
>> -  static intptr_t  atomic_xchg_long_bootstrap   (jlong, volatile 
>> jlong*);
>>
>> Wow - that looks like a bug! Only returns 32-bits on 32-bit!
> 
> This one also is only used on 64 bit.  I have to confess I don't follow 
> all the paths of the atomics and their various translations down to the 
> platform levels.   I'm not sure how windows 32 bit calls atomic_add or 
> xchg for 64 bit values (or if it is not implemented).

Not implemented at the lowest levels AFAICS - only load/store and 
cmpxchg<8>. Of course the other atomics can then be written at a 
higher-level in terms of cmpxchg.

The whole use of the stubgenerator for atomics is something that needs 
re-examining as I think it is completely unnecessary today (and likely 
for quite some time). The opening comment in atomic_windows_x86.hpp is 
somewhat startling as we approach 2018 :)

// The following alternative implementations are needed because
// Windows 95 doesn't support (some of) the corresponding Windows NT
// calls. Furthermore, these versions allow inlining in the caller.
// (More precisely: The documentation for InterlockedExchange says
// it is supported for Windows 95. However, when single-stepping
// through the assembly code we cannot step into the routine and
// when looking at the routine address we see only garbage code.
// Better safe then sorry!). Was bug 7/31/98 (gri).

Cheers,
David

>>
>> Everything else seems fine.
>>
> 
> Thanks for the eagle eye review.
> Coleen
>> Thanks,
>> David
>>
>>> Thanks,
>>> Coleen
>