> On Jul 7, 2019, at 8:08 PM, David Holmes <[email protected]> wrote:
>
> On 7/07/2019 6:48 pm, Erik Osterlund wrote:
>> The real danger is SPARC though and its BIS instructions. I don’t have the
>> code in front of me, but I really hope not to see that switch statement and
>> non-volatile loop in that pd_disjoint_words_atomic() function.
>
> sparc uses the same loop.
>
> Let's face it, almost no body expects the compiler to do these kinds of
> transformations. :(
See JDK-8131330 and JDK-8142368, where we saw exactly this sort of
transformation from a fill-loop
to memset (which may use BIS, and indeed empirically does in some cases). The
loops in question
seem trivially convertible to memcpy/memmove.
Also see JDK-8142349.
>> And I agree that the atomic copying API should be used when we need atomic
>> copying. And if it turns out the implementation of that API is not atomic,
>> it should be fixed in that atomic copying API.
>
> I agree to some extent, but we assume atomic load/stores of words all over
> the place - and rightly so. The issue here is that we need to hide the loop
> inside an API that we can somehow prevent the C++ compiler from screwing up.
> It's hardly intuitive or obvious when this is needed e.g if I simply copy
> three adjacent words without a loop could the compiler convert that to a
> block move that is non-atomic?
>
>> So I think this change looks good. But it looks like we are not done yet. :c
>
> I agree that changing the current code to use the atomic copy API to convey
> intent is fine.
I’ve been reserving Atomic::load/store for cases where the location “ought” to
be declared std::atomic<T> if
we were using C++11 atomics (or alternatively some homebrew equivalent). Not
all places where we do
stuff “atomically” is appropriate for that though (consider card tables, being
arrays of bytes, where using an
atomic<T> type might impose alignment constraints that are undesirable here).
I *think* just using volatile
here would likely be sufficient, e.g. we should have
Copy::disjoint_words_atomic(const HeapWord* from,volatile HeapWord* to,
size_t count)
