[herd] Split exclusive accesses from the X event set#1889
Conversation
|
One question I had is whether we want to remove some of the code that's used to compute |
HadrienRenaud
left a comment
There was a problem hiding this comment.
One question I had is whether we want to remove some of the code that's used to compute
atomic_load_store?
At the current state of the PR, I don't see why we would want to keep it.
| let EX = try EX with emptyset | ||
| let X = try X with emptyset | ||
|
|
||
| let lxsx = [R & EX]; po \ (po; [M & EX]; po); [W & EX] |
There was a problem hiding this comment.
This is different semantics to what it was before (where it needed to be the same memory location). IIUC this is wanted for AArch64, but is this what is wanted for the other arch?
There was a problem hiding this comment.
this is true but as we've discussed in the past, this test is allowed by the architecture:
AArch64 CoRW1+podxx
{
0:X0=x; 0:X2=y;
}
P0 ;
LDXR W1,[X0] ;
MOV W3,#1 ;
STXR W4,W3,[X2] ;
exists([y]=1)
Which I took it to mean that the requirement for same location is not valid.
There was a problem hiding this comment.
Sorry my comment was confusing. I understand that the change you made for all the architectures is wanted for AArch64. What I'm not sure is if this applies for other architectures?
In other words, I agree that this AArch64 test should behave like what your implementation does, but I'm wondering how a similar test should behave for risc-v or ppc.
There was a problem hiding this comment.
That's a good point, and I am happy to start to split out the definitions for AArch64/AArch32/arm and all other architectures. Also the case of hardware updates is very specific to AArch64 too.
There was a problem hiding this comment.
@maranget, I am not very familiar with the ordering requirements for ll/sc for other architectures. Do you think the definition of lxsx should be restricted to same location for MIPS, RISCV and PPC?
There was a problem hiding this comment.
No idea! The RISCV description clearly states that the "reserved bytes" contains (and not is equal to) the bytes written, Additionally the size of these "reserved bytes" is not specified. So I guess that the new definitiobn is acceptable for RISCV.
There was a problem hiding this comment.
Should we retain the old behaviour for architectures other than AArch64/AArch32/arm then?
There was a problem hiding this comment.
Maybe the version in the standard library needs to have the old behaviour, and aarch64utils could redefine it with the Arm-specific definition?
But at this point, I'm wondering why not keep the old computed-in-mem.ml behaviour for non-Arm architectures. And maybe at this point #1876 should be amended for simply removing the handling of implicit atomic accesses because only Arm architectures generate them.
There was a problem hiding this comment.
Happy either way. One of the problems with the old computed-in-mem.ml code is that it is also used for reasons other than computing rmw. For example, the optace optimisation relies on having rmwand then it does some checks (one of them is wrong though and started the whole discussion).
There was a problem hiding this comment.
One of the problems with the old computed-in-mem.ml code is that it is also used for reasons other than computing
rmw
Oh I had not realised. Maybe this is a good reason to keep the old computed-in-mem.ml code then?
I'm wondering if we could do a "best of both" and check that the two computations are equal in cat? This way it is explicitly defined in cat, and still enables the optimisations or the other uses of rmw before the start of the cat interpretation
Perhaps we can keep this test? |
Sorry Luc, the message is incorrect - I didn't remember correctly why I removed this test. |
|
There is a difficulty with RISCV, the following test yields no outcome at all: The reason may be that RISCV AMO instructions are represented by a specific "read and write" event. |
Thanks Luc, I wasn't really familiar with the Amo actions. I've excluded |
Commits 222e509 and 5b6af15 are the first steps in this process. Very quickly, I am stepping into the territory of other architectures and rc11.ml and I am not sure what the right thing would be. @maranget do you have a opinion about this? |
|
I guess we can get rid of dead code, BTW, there is a shadowed definition of |
| let X = try X with emptyset | ||
|
|
||
| (* Define pairs of exclusives *) | ||
| let lxsx = [R & EX]; po \ (po; [R & EX]; po); [W & EX] |
There was a problem hiding this comment.
BTW, I've relaxed this definition even further. AFAIU, for AArch64 at least, we need to pair a LDXR with the next in-po STXR (regardless of location) and any other following in-po STXR, unless we have a new LDXR.
There was a problem hiding this comment.
Is it possible that in an instruction sequence LDXR; STXR; STXR; the second STXR may succeed? I thought that STXR was cancelling any reservation, whether it succeeds or not.
There was a problem hiding this comment.
And, as far as I know, this is what our semantics does.
There was a problem hiding this comment.
Sorry I think I got confused and I will revert this.
If the STXR is successful then the global monitor has to to go to open access: https://developer.arm.com/documentation/ddi0487/mb/-Part-B-The-AArch64-Application-Level-Architecture/-Chapter-B2-The-AArch64-Application-Level-Memory-Model/-B2-12-Synchronization-and-semaphores/-B2-12-1-Exclusive-access-instructions-and-Non-shareable-memory-locations . In our case the presence of an Explicit Memory Write Effect is the consequence of a successful STXR.
|
FYI I have made a draft PR #1894 that computes the EX and X sets in OCaml differently. The main feature is a (hopfully) fixed computation of rmw in OCaml. Namely, the computation of PR #1876 is incorrect by lack of making the difference between exclusives and AMO's. The PR also adds a predicate |
|
|
||
| (* Internal visibility requirement *) | ||
| acyclic po-loc | ca | rf as internal | ||
| acyclic po-loc | amo | ca | rf as internal |
There was a problem hiding this comment.
You may consider applying the same fix to x86tso.cat, which is the default model of the architecture X86. We may also add at lease the corresponding test for this (obsolete I admit) architecture:
X86 CoRW2+rmw
{
}
P0 | P1 ;
MOV EAX,$1 | MOV [x],$2 ;
XCHG [x],EAX | ;
exists ([x]=2 /\ 0:EAX=2)
maranget
left a comment
There was a problem hiding this comment.
LGTM. Some suggestions for a future PR
- Add LR/SC and AMO tests for all architectures.
- As long as there is is_atomic on events, we can define teh set "X" in machModelChecker.ml and avoid arch-specific defintions.
Wouldn't it be preferable to move towards a solution where the |
|
@maranget @HadrienRenaud are you happy with this PR? If you are, I'll rebase squash and merge. |
Both solutions are fine (although mutually exclusive...). The first one (in MachModelChecker.ml) is compatible with the idea that such "atomic" accesses are a common to all architectures and avoids some duplication of code. The second (in semantics)n highlights the connexion with semantics. |
I'd like to have a last look at the last commit. |
|
Hi @relokin. LGTM, merge at your earliest convenience. |
HadrienRenaud
left a comment
There was a problem hiding this comment.
This looks good to me, thanks.
Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
|
Merged, thanks @maranget and @HadrienRenaud ! |
This commit regroups three changes related to the new organisation of "atomic" accesses performed by PR #1889: such effects are now partitioned into the sets 'X' (emited by access-modify instructions, OCaml function `is_atomic`) and 'EX' (emited by exclusive access instructions, _i.e._ load reserve and store conditional, OCaml function `is_exclusive`). Notice that before the OCaml function `is_atomic` and the Cat set `X` were gathering all such effects. Also notice that `is_exclusive` was absent and that there existed a set `AMO` gathering the memory write effects emited by access-modify instructions. 1. Noticing that `is_atomic` no longer holds on effects generated by exclusive load and store, change `collect_atomics` (from memUtils.ml) names and code into `collect_atomics_and_exclusives`. 2. Modify the RISCV Cat models to account for the new semantics of the sets 'X' and `EX`. 3. Remove the Cat set `AMO` and the code that computed it.
This commit regroups three changes related to the new organisation of "atomic" accesses performed by PR #1889: such effects are now partitioned into the sets 'X' (emited by access-modify instructions, OCaml function `is_atomic`) and 'EX' (emited by exclusive access instructions, _i.e._ load reserve and store conditional, OCaml function `is_exclusive`). Notice that before the OCaml function `is_atomic` and the Cat set `X` were gathering all such effects. Also notice that `is_exclusive` was absent and that there existed a set `AMO` gathering the memory write effects emited by access-modify instructions. 1. Noticing that `is_atomic` no longer holds on effects generated by exclusive load and store, change `collect_atomics` (from memUtils.ml) names and code into `collect_atomics_and_exclusives`. 2. Modify the RISCV Cat models to account for the new semantics of the sets 'X' and `EX`. 3. Remove the Cat set `AMO` and the code that computed it.
This commit regroups three changes related to the new organisation of "atomic" accesses performed by PR #1889: such effects are now partitioned into the sets 'X' (emited by access-modify instructions, OCaml function `is_atomic`) and 'EX' (emited by exclusive access instructions, _i.e._ load reserve and store conditional, OCaml function `is_exclusive`). Notice that before the OCaml function `is_atomic` and the Cat set `X` were gathering all such effects. Also notice that `is_exclusive` was absent and that there existed a set `AMO` gathering the memory write effects emited by access-modify instructions. 1. Noticing that `is_atomic` no longer holds on effects generated by exclusive load and store, change `collect_atomics` (from memUtils.ml) names and code into `collect_atomics_and_exclusives`. 2. Modify the RISCV Cat models to account for the new semantics of the sets 'X' and `EX`. 3. Remove the Cat set `AMO` and the code that computed it.
This commit regroups three changes related to the new organisation of "atomic" accesses performed by PR #1889: such effects are now partitioned into the sets 'X' (emited by access-modify instructions, OCaml function `is_atomic`) and 'EX' (emited by exclusive access instructions, _i.e._ load reserve and store conditional, OCaml function `is_exclusive`). Notice that before the OCaml function `is_atomic` and the Cat set `X` were gathering all such effects. Also notice that `is_exclusive` was absent and that there existed a set `AMO` gathering the memory write effects emited by access-modify instructions. 1. Noticing that `is_atomic` no longer holds on effects generated by exclusive load and store, change `collect_atomics` (from memUtils.ml) names and code into `collect_atomics_and_exclusives`. 2. Modify the RISCV Cat models to account for the new semantics of the sets 'X' and `EX`. 3. Remove the Cat set `AMO` and the code that computed it.
[herd,cat] Fix some problems with the C architecture This PR fixes two problems: 1. Since PR #1855, the polymorphic value `V.Empty` circulates more. Some primitives were performing set and relation normalisation by themselves, overlooking the cases of empty and universe. We fix this by using the already present and complete normalisation functions `as_set` and `as_rel` slightly generalised to handle different error reporting. 2. PR #1889 changed how the relation `rmw` is computed by defining it in stdlb.cat. The Cat definition of the relation `rmw` assumes that atomic effects are gathered in the set `X`, which was not the case of the C semantics. As a result the `rmw` relation was empty when the read-modify-write instructions are implemented by two (atomic) effects.
This commit regroups three changes related to the new organisation of "atomic" accesses performed by PR #1889: such effects are now partitioned into the sets 'X' (emited by access-modify instructions, OCaml function `is_atomic`) and 'EX' (emited by exclusive access instructions, _i.e._ load reserve and store conditional, OCaml function `is_exclusive`). Notice that before the OCaml function `is_atomic` and the Cat set `X` were gathering all such effects. Also notice that `is_exclusive` was absent and that there existed a set `AMO` gathering the memory write effects emited by access-modify instructions. 1. Noticing that `is_atomic` no longer holds on effects generated by exclusive load and store, change `collect_atomics` (from memUtils.ml) names and code into `collect_atomics_and_exclusives`. 2. Modify the RISCV Cat models to account for the new semantics of the sets 'X' and `EX`. 3. Remove the Cat set `AMO` and the code that computed it.
[herd] Consequences of splitting exclusive effects from the X events set (PR #1889) This PR groups three commits 1. Fixing the RISCV model (and the `collect_atomics` function from memUtils.ml) 2. Add some RISCV tests that check the problem fixed in 1. 3. Fix the computation of "atomic pairs" in OCaml, this computation was wrong before PR #1889: the cases of AMO's and exclusive accessed were not separated as they should be. As to 3., notice that, since PR #1889, there is no need to compute atomic pairs in OCaml for AArch64 and possibly for all Cat model (perhaps at the price of a small optimisation of `-optace iico`). Implementation of limiting the usage of the OCaml code for atomic pairs not being that straightforward, we leave that to a further PR.
Summary
This PR makes exclusive accesses explicit in
herdby introducing a distinctEXevent set instead of treating them aspart of
X.The main goal is to let CAT models distinguish reservation-based exclusive operations from other atomic/RMW events more
precisely, while also exposing reusable helpers for reasoning about RMWs.
What changed
EX,EXA,EXL) and thread them through the AArch64/ASL semanticsEXfor exclusive/LR-SC style instructions in ARM, RISC-V, MIPS, and PPCXseparate fromEX, rather than conflating all of these events in one setstdlib.catwith reusableEX,lxsx,amo, andrmwhelpersppc-checks.catsoscXXis defined over exclusive eventsA162test which duplicatesA44Why
Previously, exclusive accesses were folded into the generic
Xset. That made it harder to express model rules thatspecifically target reservation-based exclusives rather than all atomic accesses.
By separating
EXfromX, the model becomes clearer and CAT code can talk about exclusives and RMWs directly.