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

[David Holmes]

On 8/12/2017 1:28 AM, coleen.phillimore at oracle.com wrote:
> On 12/7/17 8:11 AM, coleen.phillimore at oracle.com wrote:
>>
>>
>> On 12/6/17 11:33 PM, David Holmes wrote:
>>> 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!)
>>
>> Yes, all the ptrs variants were removed in a previous change but I'd 
>> missed this renaming this one to long.  Since I was fixing the 
>> arguments for this function, I took the opportunity to also change the 
>> name in the same line.  I could file a new RFE for this and separate 
>> out the few lines.  The _long_ matches the other function names, and 
>> this case is actually used on _long_ because by this point the 
>> pointers have been normalized by template magic to long when they get 
>> here.   See the other related functions in this change:
> 
> Sorry, I meant _long_ matches other function names and this case is 
> actually used on int64_t because the pointers have been normalized to 8 
> byte int64_t, not _long_ because _long_ is 32 bits on windows.   So much 
> confuse.

Thanks for clarifying that. :)

No further comments from me. I hope all these "long" variants can 
disappear in the near future. Along with stub generator use in Atomic :)

Thanks,
David

> thanks,
> Coleen
> 
>>
>> http://cr.openjdk.java.net/~coleenp/8186903.01/webrev/src/hotspot/os_cpu/windows_x86/os_windows_x86.hpp.udiff.html 
>>
>>>
>>>> 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 :)
>>
>> I asked about this in the review and Erik said they were needed for 
>> some reason, but it would be nice to have these cleaned up somehow. 
>> Kim has some ideas for simplifying the implementation. My script to 
>> find all these names goes something like this:
>>
>> foreach atomic (cmpxchg xchg add load store)
>> ...
>>      foreach type (entry func bootstrap)
>> ...
>>           foreach ("" "long" "byte")
>> ...
>>
>>>
>>> // 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).
>>
>> Yeah, good thing we can single step on Windows 95!
>>
>> Thanks,
>> Coleen
>>
>>>
>>> Cheers,
>>> David
>>>
>>>>>
>>>>> Everything else seems fine.
>>>>>
>>>>
>>>> Thanks for the eagle eye review.
>>>> Coleen
>>>>> Thanks,
>>>>> David
>>>>>
>>>>>> Thanks,
>>>>>> Coleen
>>>>
>>
>