Add support for C26 atomic reductions (without compiler mappings)#985
Add support for C26 atomic reductions (without compiler mappings)#985ThomasHaas wants to merge 9 commits intodevelopmentfrom
Conversation
15e1a9c to
dc83233
Compare
dc83233 to
ad06c85
Compare
|
@graymalkin this branch should have everything you need to play around with the model |
|
FYI, |
|
Thanks, I'll check it out! |
f33d0a3 to
2cadfc2
Compare
|
Code-wise I think this one is ready to merge. I will wait a few days to see if @graymalkin or @gonzalobg have comments about the memory model part (especially if it makes sense to mark the read part of the reduction as atomic) or @mmalcomson reports any issues when trying the code. |
|
With #986 merged, we could in principle add compiler mappings for atomic reductions to armv8. At least the obvious one's like |
dartagnan/src/main/java/com/dat3m/dartagnan/program/event/Tag.java
Outdated
Show resolved
Hide resolved
7be51db to
1ca78bc
Compare
| Local localOp = newLocal(dummyReg, expressions.makeIntBinary(dummyReg, e.getOperator(), e.getOperand())); | ||
| RMWStore store = newRMWStoreWithMo(load, address, dummyReg, Tag.C11.storeMO(mo)); | ||
|
|
||
| load.addTags(C11.ATOMIC, Tag.C11.NORETURN); // Note that the load has no mo, but is still atomic! |
There was a problem hiding this comment.
For consistency with visitAtomicFetchOp I would rather use
Load load = newRMWLoadWithMo(dummyReg, address, Tag.C11.loadMO(mo));
and rather than getting the expected ordering guarantees "by chance" as it currently happens for rc11,
let the model explicitly state if NORETURN events should provide order or not.
It also feels strange to have an atomic event with no memory order.
There was a problem hiding this comment.
I don't like these consistency arguments... those are different operations. Tag.C11.loadMO(mo) will just be RLX or SC because you cannot specify ACQ/ACQ_REL in the first place.
I think the only really sensible options are: the load has no mo, simply because it shouldn't exist in the first place, or the load has the same mo/tags as the store and the WMM removes the tags.
Anything inbetween seems arbitrary to me.
There was a problem hiding this comment.
The current solution of hardcoding the atomic tag seems equally arbitrary.
I guess what you are proposing is to completely get rid of Tag.C11.loadMO/storeMO) and simply used the mo from the parsing. This would require the memory model to do some "cleanup" as lkmm does, but then we can get rid of these loadMo/storeMO as we already did for lkmm in #893.
There was a problem hiding this comment.
The current solution of hardcoding the atomic tag seems equally arbitrary.
The atomic tag is not arbitrary, because the whole operation is an atomic one, even by name atomic_store_XYZ.
And if you look at what our compiler does:
boolean canRace = mo == null || mo.value().equals(C11.NONATOMIC);
e.addTags(canRace ? C11.NONATOMIC : C11.ATOMIC);then every event must be tagged either way, and NONATOMIC is certainly more wrong than ATOMIC.
I guess what you are proposing is to completely get rid of
Tag.C11.loadMO/storeMO)and simply used themofrom the parsing. This would require the memory model to do some "cleanup" as lkmm does, but then we can get rid of theseloadMo/storeMOas we already did for lkmm in #893.
I proposed exactly that in #984 or rather suggested it as one possible way to go forward. I think rc11.cat might already adhere to that. That being said, for now, I just took the most natural solution given the current hardcoded one:
- A load must be generated for data-flow modelling (no way around this)
- The load must be ignored in data races. Marking it as atomic is natural as it is part of an atomic operation independent of its memory ordering.
- The load should not provide any orderings -> both plain (no mo) and RLX seem reasonable. Plain is closer to capturing the idea of "the load should not exist" whereas RLX is closer to capturing the idea of "the load exists but it should not give orderings", which is (funnily enough) too much ordering :)
At the end of the day, I'm not the one who writes the C memory models and sets the expectation of what is assumed to happen implicitly and what is assumed to be done in the model.
1ca78bc to
db502d6
Compare
Implemented first support for atomic reduction ops (only for C-code yet).
…y are have no memory order (~plain).
Signed-off-by: Hernan Ponce de Leon <hernanl.leon@huawei.com>
db502d6 to
2a5fa5f
Compare
| }; | ||
| } | ||
|
|
||
| public static String intToMo(int i) { |
There was a problem hiding this comment.
Why do we need this back?
There was a problem hiding this comment.
It is used in our Intrinsics to get the correct memory ordering for the new atomic reductions from our custom c26 header. I think once LLVM supports those instructions natively, we won't need this anymore.
EDIT: I could move the mapping code into Intrinsics if you prefer.
There was a problem hiding this comment.
I could move the mapping code into
Intrinsicsif you prefer.
That might be better. Also, please add a TODO so we remember to get rid of this once LLVM supports the instructions.
There was a problem hiding this comment.
Once LLVM supports those instructions, we will get parser issues anyhow :). The code needs to change, so you cannot forget it really.
Add comments in c26.h
c26.hwith the new atomic reduction operations of C26.I implemented all of them exceptminandmaxmin/maxare supported now, but only the signed versions!These atomics generate a rmw-pair of events just like a standard fetch_op atomic, but add the
Noreturntag to both of them (naming follows LKMM's non-returning atomics).There is no compilation scheme to hardware targets yet, so code has to be verified with
--target=c11(default).What needs to be done is to relax the memory models of interest:
right nowEDIT: Although the memory models should probably be adapted, the fact that we currently model the load part ofatomic_opandatomic_fetch_opprovide the same synchronization semantics.atomic_store_opas a plain load (not even relaxed) makes it weaker than aatomic_fetch_opin terms of ordering.