-
Notifications
You must be signed in to change notification settings - Fork 37
Add support for C26 atomic reductions (without compiler mappings) #985
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
ThomasHaas
wants to merge
7
commits into
development
Choose a base branch
from
atomic-modify-write
base: development
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
c6c73c8
Prototype support for atomic modify-write operations
hernan-poncedeleon 413b210
Add c26 header for atomic (reduction) ops
ThomasHaas ba69de3
Rename C26 reduction operators
ThomasHaas dfea4c6
Add min/max operators to C
ThomasHaas 38cdfdd
Make the loads of atomic_store_op compilation atomic, even though the…
ThomasHaas 497ae55
Fix accidental compilation error
ThomasHaas 1ca78bc
Noreturn -> NORETURN
hernan-poncedeleon File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
37 changes: 37 additions & 0 deletions
37
dartagnan/src/main/java/com/dat3m/dartagnan/program/event/lang/catomic/AtomicOp.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| package com.dat3m.dartagnan.program.event.lang.catomic; | ||
|
|
||
| import com.dat3m.dartagnan.expression.Expression; | ||
| import com.dat3m.dartagnan.expression.integers.IntBinaryOp; | ||
| import com.dat3m.dartagnan.program.Register; | ||
| import com.dat3m.dartagnan.program.event.EventVisitor; | ||
| import com.dat3m.dartagnan.program.event.common.RMWOpBase; | ||
| import com.dat3m.dartagnan.program.event.Tag; | ||
| import com.google.common.base.Preconditions; | ||
|
|
||
| public class AtomicOp extends RMWOpBase { | ||
|
|
||
| public AtomicOp(Expression address, IntBinaryOp operator, Expression operand, String mo) { | ||
| super(address, operator, operand, mo); | ||
| Preconditions.checkArgument(!mo.isEmpty(), "Atomic events cannot have empty memory order"); | ||
| addTags(Tag.C11.NORETURN); | ||
| } | ||
|
|
||
| private AtomicOp(AtomicOp other) { | ||
| super(other); | ||
| } | ||
|
|
||
| @Override | ||
| public String defaultString() { | ||
| return String.format("atomic_store_%s(*%s, %s, %s)\t### C11", operator.getName(), address, operand, mo); | ||
| } | ||
|
|
||
| @Override | ||
| public AtomicOp getCopy() { | ||
| return new AtomicOp(this); | ||
| } | ||
|
|
||
| @Override | ||
| public <T> T accept(EventVisitor<T> visitor) { | ||
| return visitor.visitAtomicOp(this); | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
|
|
||
| typedef enum { | ||
| __C26_op_add, | ||
| __C26_op_sub, | ||
| __C26_op_and, | ||
| __C26_op_or, | ||
| __C26_op_xor, | ||
| __C26_op_min, | ||
| __C26_op_max, | ||
| } __C26_op; | ||
|
|
||
| #define __C26__DECLARE_ATOMIC_OP(type) \ | ||
| extern void __C26_atomic_op_##type(_Atomic(type)* ptr, type value, __C26_op, memory_order); | ||
|
|
||
| __C26__DECLARE_ATOMIC_OP(short) | ||
| __C26__DECLARE_ATOMIC_OP(int) | ||
| __C26__DECLARE_ATOMIC_OP(long) | ||
|
|
||
| #define __SELECT(p) _Generic((p), \ | ||
| _Atomic(short)* : __C26_atomic_op_short, \ | ||
| _Atomic(int)* : __C26_atomic_op_int, \ | ||
| _Atomic(long)* : __C26_atomic_op_long \ | ||
| ) | ||
|
|
||
| // NOTE: for min/max we only have the signed version for now. | ||
|
|
||
| #define atomic_store_add_explicit(p, v, mo) __SELECT(p)(p, v, __C26_op_add, mo) | ||
| #define atomic_store_sub_explicit(p, v, mo) __SELECT(p)(p, v, __C26_op_sub, mo) | ||
| #define atomic_store_and_explicit(p, v, mo) __SELECT(p)(p, v, __C26_op_and, mo) | ||
| #define atomic_store_or_explicit(p, v, mo) __SELECT(p)(p, v, __C26_op_or, mo) | ||
| #define atomic_store_xor_explicit(p, v, mo) __SELECT(p)(p, v, __C26_op_xor, mo) | ||
| #define atomic_store_min_explicit(p, v, mo) __SELECT(p)(p, v, __C26_op_min, mo) | ||
| #define atomic_store_max_explicit(p, v, mo) __SELECT(p)(p, v, __C26_op_max, mo) | ||
|
|
||
| #define atomic_store_add(p, v) __SELECT(p)(p, v, __C26_op_add, memory_order_seq_cst) | ||
| #define atomic_store_sub(p, v) __SELECT(p)(p, v, __C26_op_sub, memory_order_seq_cst) | ||
| #define atomic_store_and(p, v) __SELECT(p)(p, v, __C26_op_and, memory_order_seq_cst) | ||
| #define atomic_store_or(p, v) __SELECT(p)(p, v, __C26_op_or, memory_order_seq_cst) | ||
| #define atomic_store_xor(p, v) __SELECT(p)(p, v, __C26_op_xor, memory_order_seq_cst) | ||
| #define atomic_store_min(p, v) __SELECT(p)(p, v, __C26_op_min, memory_order_seq_cst) | ||
| #define atomic_store_max(p, v) __SELECT(p)(p, v, __C26_op_max, memory_order_seq_cst) |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency with
visitAtomicFetchOpI would rather useand rather than getting the expected ordering guarantees "by chance" as it currently happens for rc11,
let the model explicitly state if
NORETURNevents 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like these consistency arguments... those are different operations.
Tag.C11.loadMO(mo)will just beRLXorSCbecause you cannot specifyACQ/ACQ_RELin 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
then every event must be tagged either way, and
NONATOMICis certainly more wrong thanATOMIC.I proposed exactly that in #984 or rather suggested it as one possible way to go forward. I think
rc11.catmight already adhere to that. That being said, for now, I just took the most natural solution given the current hardcoded one: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.