-
Notifications
You must be signed in to change notification settings - Fork 110
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
[CIR][CIRGen][Builtin] Support __sync_add_and_fetch #1077
Conversation
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.
Also, didn't implement invert as __sync_add_and_fetch does not need it
What do you mean?
|
||
// These output arguments are needed for post atomic fetch operations | ||
// that calculate the result of the operation as return value of | ||
// <binop>_and_fetch builtins. The `AtomicFetch` operation ony updates the |
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.
*only
OG has additional argument
I didn't implement them so to keep test coverage , but will implement them when we implement __sync_nand_and_fetch which seems to be only case for this logic. |
✅ With the latest revision this PR passed the C/C++ code formatter. |
Where's the missing feature assert + comments talking about that in code? |
|
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.
One minor conflict, but otherwise LGTM
Notable change is to introduce helper function
buildBinaryAtomicPost
which models on OG'sEmitBinaryAtomicPost
. Comparing toEmitBinaryAtomicPost
,buildBinaryAtomicPost
is more concise as OG'sEmitBinaryAtomicPost
duplicates quite a bit of code from MakeBinaryAtomicValueAlso, didn't implement invert as __sync_add_and_fetch does not need it, but will add it (which is a trivial work) when we implement a builtin that needs it.
Test cases are from OG