On 2/12/2014 6:46 AM, Martin Buchholz wrote:
David, Paul (i.e. Reviewers) and Doug,
I'd like to commit corrections so we make progress.
I think the current webrev is simple progress with the exception of my
attempt to translate volatiles into fences, which is marginal (but was
a good learning exercise for me).
Looking at the actual API changes ...
In general phrasing like: " also known as a LoadLoad plus LoadStore
barrier ..." is misleading to me as these are not "aliases"- the
loadFence (in this case) is being specified to have the same semantics
as the loadload|storeload. It should say "corresponds to a LoadLoad plus
LoadStore barrier" - as per the "Corresponds to a C11 ...". And
referring to things like "load-acquire fence" is meaningless without
some reference to a definition - who defines a load-acquire fence? Is
there a universal definition? I would be okay with something looser eg:
/**
* Ensures that loads before the fence will not be reordered with
loads and
* stores after the fence. Corresponds to a LoadLoad plus LoadStore
barrier,
* and also to the C11 atomic_thread_fence(memory_order_acquire).
* Sometimes referred to as a "load-acquire fence".
*
Also I find this comment strange:
! * A pure StoreStore fence is not provided, since the addition of
LoadStore
! * is almost always desired, and most current hardware
instructions that
! * provide a StoreStore barrier also provide a LoadStore barrier
for free.
because inside hotspot we use storeStore barriers a lot, without any
loadStore at the same point.
David
