RFR(S): 8229422: Taskqueue: Outdated selection of weak memory model platforms

[David Holmes]

Hi Martin,

On 30/08/2019 9:14 pm, Doerr, Martin wrote:
> Hi Thomas,
> 
> good proposal.
> 
> Here's the minimal version:
> http://cr.openjdk.java.net/~mdoerr/8229422_multi-copy-atomic/webrev.02/
> 
> I've removed the compiler part. I can create a separate issue for making C1 and C2 consistent.
> 
> Arm32/aarch64 folks can create new issues if they like further changes.
> I don't have any further requirements for s390 and PPC64 at the moment.
> 
> Can I consider it as reviewed by Thomas, David and Derek?

The Aarch64 comment begs the question as to why it is not defining 
CPU_MULTI_COPY_ATOMIC, but otherwise the changes are okay. My suggestion 
for Aarch64 would be:

// Aarch64 was not originally defined as multi-copy-atomic, but now is.
// See: "Simplifying ARM Concurrency: Multicopy-atomic Axiomatic and 
Operational Models for ARMv8"
// So we could #define CPU_MULTI_COPY_ATOMIC but historically we have 
not done so.

Thanks,
David

> Best regards,
> Martin
> 
> 
>> -----Original Message-----
>> From: Thomas Schatzl <thomas.schatzl at oracle.com>
>> Sent: Freitag, 30. August 2019 10:33
>> To: Doerr, Martin <martin.doerr at sap.com>; hotspot-runtime-
>> dev at openjdk.java.net; 'hotspot-compiler-dev at openjdk.java.net' <hotspot-
>> compiler-dev at openjdk.java.net>
>> Cc: hotspot-gc-dev at openjdk.java.net; David Holmes
>> (david.holmes at oracle.com) <david.holmes at oracle.com>; Derek White
>> <derekw at marvell.com>
>> Subject: Re: RFR(S): 8229422: Taskqueue: Outdated selection of weak
>> memory model platforms
>>
>> Hi,
>>
>> On 26.08.19 15:04, Doerr, Martin wrote:
>>> Hi all,
>>>
>>> I had noticed that the platforms selection which need a fence in
>> taskqueue.inline.hpp should get updated.
>>>
>>> My initial webrev
>>> http://cr.openjdk.java.net/~mdoerr/8229422_multi-copy-
>> atomic/webrev.00/
>>> was already reviewed on hotspot-gc-dev. It is an attempt to make things
>> more consistent, especially the property "CPU_MULTI_COPY_ATOMIC".
>>> Also the compiler constant
>> "support_IRIW_for_not_multiple_copy_atomic_cpu" depends on this
>> property (currently only used on PPC64).
>>>
>>> We could go one step further and move even more #defines into the
>> platform files to give platform maintainers more control.
>>> I haven't got feedback from arm/aarch64 folks about this addition, yet:
>>> http://cr.openjdk.java.net/~mdoerr/8229422_multi-copy-
>> atomic/webrev.01/
>>> With this proposal, each platform which is "CPU_MULTI_COPY_ATOMIC" is
>> supposed to define this macro.
>>> Other platforms must define
>> SUPPORT_IRIW_FOR_NOT_MULTI_COPY_ATOMIC_CPU and
>> IRIW_WITH_RELEASE_VOLATILE_IN_CONSTRUCTOR for fine-grained control
>> of the memory ordering behavior.
>>> We can even control them dynamically (added an experimental switch for
>> PPC64 as an example).
>>>
>>> Note that neither webrev.00 nor webrev.01 contain any functional changes
>> other than the taskqueue update for s390 (and the experimental switch for
>> PPC64 in webrev.01).
>>>
>>> Feedback is welcome. Also if you have a preference wrt. webrev.00 vs.
>> webrev.01.
>>
>>     for pushing I would prefer the minimal amount of changes to solve the
>> original issue, and move all other changes to a different CR.
>>
>> Also, I would prefer if all globalDefinitions files contained all
>> defines, commented out if needed. I.e. to try to show that not defining
>> a particular macro has been deliberate and not an oversight.
>>
>> (Like in the 00 webrev where the code at least states for aarch64:
>> 37 // aarch64 is not CPU_MULTI_COPY_ATOMIC
>>
>> I am aware that this is not correct given new information, but in
>> context of the CR it is/was)
>>
>> Further, let's avoid "TODOs" in the sources, the correct place for those
>> is JIRA imho. :)
>>
>> Thanks,
>>     Thomas
>>
>