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

[David Holmes]

Hi Martin,

On 13/08/2019 8:30 pm, Doerr, Martin wrote:
> Hi Kim and David,
> 
> thank you for looking into this issue.
> 
> @Kim:
> I've replied to your comment in the issue.
> 
> @David:
>> I find the inversion of the ifdef slightly confusing. I also don't like
>> a comment to say we don't have a given property. Wouldn't it be better
>> to set CPU_MULTI_COPY_ATOMIC to 0 or 1 as appropriate?
> Hmm. We could change that. I'm not sure what is better.
> I think it should be designed such that correct usage is easy and wrong usage is difficult.
> 
> It has already happened that people used an #ifdef for a macro which is always defined (0 or 1) by mistake.
> That's why I'm not a big fan of defining things to 0 or 1.
> 
> With the #define or not define approach, all platforms except those which explicitly specify the property are conservatively treated as non-multi-copy atomic.

If you need to put a comment saying "we don't have this property" then 
to me that means there should be something more than a comment to 
indicate that.

> But if your version is preferred by all reviewers, I can use it.

I'll defer to others/majority.

> 
>> Can't comment on ppc64 specifics.
> I'll ask for additional reviews once the main issue was reviewed.
> 
> 
>> It's not at all obvious to me that the need for the fence() in
>> pop_global is directly related to CPU_MULTI_COPY_ATOMIC. I prefer to see
>> that define connected only with the IRIW issue as it currently is.
> This was explained in the email thread a few emails later:
> http://mail.openjdk.java.net/pipermail/hotspot-dev/2013-March/008853.html

Okay I refreshed my memory. Yes the fence() does relate to 
non-multi-copy-atomic systems. But we were only using the 
CPU_NOT_MULTIPLE_COPY_ATOMIC define to control the setting of 
support_IRIW_for_not_multiple_copy_atomic_cpu. I don't know why we added 
the cpu-based ifdefs around the fence() rather than using 
CPU_NOT_MULTIPLE_COPY_ATOMIC in the first place, but it use for that 
does seem valid.

Thanks,
David


> 
> Best regards,
> Martin
>