(Spun off from bug 1714367) For a plain "atomic store 37" in isolation we have eg this (Ion code): ``` <alignment guard> 0x268f321d2040 528004a1 mov w1, #0x25 0x268f321d2044 d5033abf dmb ishst 0x268f321d2048 b8206aa1 str w1, [x21, x0] 0x268f321d204c d5033bbf dmb ish ``` `dmb ishst` is slightly weaker than just a `dmb ish`. The intent of the `dmb ishst; str; dmb ish` is to implement a store-release operation, which is what we really want here (see bug 1442534, but that's for later). The question is, is this the correct implementation, or should the first fence also be `dmb ish`? After some searching I have concluded that "it's complicated". The OpenJDK HotSpot compiler used to have the same code as we do, with `dmb ishst` before a volatile store and a `dmb ish` after. In [this paper](https://dl.acm.org/doi/abs/10.1145/3314221.3314611) (page "19", near top) it is on the one hand stated that this is "overly weak" for volatile semantics; in the same paragraph it is then stated that it is nevertheless "sufficient" for reasons that are essentially the same as ours, namely, every op ends with a `dmb ish` anyway and establishes the necessary relation; and a footnote explains that the HotSpot maintainers accepted a change that changed the `ishst` to an `ish` (see [here](http://hg.openjdk.java.net/jdk/jdk/rev/e2fc434b410a)). Maybe the confusion stems from two separate products being mentioned, standard JVM HotSpot and a volatile-all-the-things research project, which has slightly different semantics. In short, I think that the code we have is probably correct, but since there is only the tiniest edge to be gained from this we should change our codegen so that there's no doubt, until we can switch to the better ARM64 instructions.
Bug 1715494 Comment 0 Edit History
Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.
(Spun off from bug 1714367) For a plain "atomic store 37" in isolation we have eg this (Ion code): ``` <alignment guard> 0x268f321d2040 528004a1 mov w1, #0x25 0x268f321d2044 d5033abf dmb ishst 0x268f321d2048 b8206aa1 str w1, [x21, x0] 0x268f321d204c d5033bbf dmb ish ``` `dmb ishst` is slightly weaker than just a `dmb ish`. The intent of the `dmb ishst; str; dmb ish` is to implement a store-release operation, which is what we really want here (see bug 1442534, but that's for later). The question is, is this the correct implementation, or should the first fence also be `dmb ish`? After some searching I have concluded that "it's complicated". The OpenJDK HotSpot compiler used to have the same code as we do, with `dmb ishst` before a volatile store and a `dmb ish` after. In [this paper](https://dl.acm.org/doi/abs/10.1145/3314221.3314611) (page "19", near top) it is on the one hand stated that this is "overly weak" for volatile semantics; in the same paragraph it is then stated that it is nevertheless "sufficient" for reasons that are essentially the same as ours, namely, every op ends with a `dmb ish` anyway and establishes the necessary relation; and a footnote explains that the HotSpot maintainers accepted a change that changed the `ishst` to an `ish` (see [here](http://hg.openjdk.java.net/jdk/jdk/rev/e2fc434b410a) and [here](https://bugs.openjdk.java.net/browse/JDK-8218185)). Maybe the confusion stems from two separate products being mentioned, standard JVM HotSpot and a volatile-all-the-things research project, which has slightly different semantics. In short, I think that the code we have is probably correct, but since there is only the tiniest edge to be gained from this we should change our codegen so that there's no doubt, until we can switch to the better ARM64 instructions.