RFR: 8253843: AArch64: Use ishst for storestore barrier

[Alan Hayward]

On Thu, 1 Oct 2020 01:38:03 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> I'm also getting CI failures unrelated to this patch, due to disks being full:
>> https://github.com/a74nh/jdk/actions/runs/280072223
>
> 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?

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

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