RFR: 8253843: AArch64: Use ishst for storestore barrier

[Alan Hayward]

On Fri, 2 Oct 2020 08:56:29 GMT, Alan Hayward <github.com+4146708+a74nh at openjdk.org> wrote:

>> PRs for mainline should be reviewed on mainline mailing lists. The xxx-port mailing lists are supposed to be used for
>> changes applied to port specific repositories, hence the is no label mapping for them here. You can of course reply
>> directly to the PR email and add the additional mailing list.
>
>> _Mailing list message from [Andrew Haley](mailto:aph at redhat.com) on
>> [hotspot-runtime-dev](mailto:hotspot-runtime-dev at openjdk.java.net):_
>> On 30/09/2020 17:29, Alan Hayward wrote:
>> 
>> > AArch64 orderAccess uses gcc built in atomic functions, which expand
>> > inline to DMB barrier instructions.  Specifically, they call the following:
>> > FULL_MEM_BARRIER -> DMB ISH
>> > READ_MEM_BARRIER -> DMB ISHLD
>> > WRITE_MEM_BARRIER -> DMB ISH
>> > However, storestore should be optimised to use ISHST.
>> > In addition, __sync_synchronize is marked as legacy. See:
>> > https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html
>> > In order for the code to match,
>> 
>> To match what?
> 
> Original version of my patch just switched storestore to call asm dmb ishst
> But then it looks very odd that one function in the file is using asm and the
> rest of the file is using the C++ functions.
> 
>> 
>> > I switched everything to call dmbs directly.
>> 
>> We are now able to use modern C++, which means that we can use real
>> C++ atomic operations. Going back to inline asms would be a
>> regression.
>> 
>> The trouble with using asms for this is that the compiler does not
>> know what is going on inside an asm. If you use an asm with a memory
>> clobber for a StoreStore barrier, the compiler has to treat the asm as
>> a full memory barrier, so it cannot move any loads and stores across
>> the StoreStore. So, paradoxically, you might be making things worse.
>> 
> 
> Ok, I agree with that. (maybe a comment in the code would be useful here)
> 
> At the risk of asking something that's probably been asked before ....Why
> then do we have inline asm for other targets? Wouldn't it be better to
> have a common (or OS?) level file for these functions?
> 
> Also, it looks like using the C++ atomic functions there seems to be no way to
> generate a dmb ishst.
> 
>> > Also, add AArch64 to the orderAccess documentation table.
>> 
>> Please don't. it's more complicated than that, and it's not necessary.
>> 
> 
> Happy to drop that. But, if it is more complicated than that, then is it wrong
> for other architectures too? Should that whole table be removed?

Closing this. Inline asm is the wrong way to go; it's not sensible to commonise everything else to the c++ functions;
there's no C++ atomic function for ishst.

-------------

PR: https://git.openjdk.java.net/jdk/pull/427