-
Notifications
You must be signed in to change notification settings - Fork 21
Conversation
96a6c5d
to
6a6a29e
Compare
@@ -1237,6 +1237,16 @@ isReMaterializable = 1 in | |||
Requires<[HasSRAM]>; | |||
} | |||
|
|||
def AtomicLoad8 : Pseudo<(outs GPR8:$rd), | |||
(ins PTRREGS:$rr), | |||
"atomic_laod $rd, $rr", |
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.
Typo: laod
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.
nice catch!
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.
Have fixed this up and force pushed.
89f4777
to
e9b4a6e
Compare
|
||
template<> | ||
bool AVRExpandPseudo::expand<AVR::AtomicLoad8>(Block &MBB, BlockIt MBBI) { | ||
return expandAtomicBinaryOp(AVR::MOVRdRr, MBB, MBBI); |
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.
Should we lower this to a MOV or an LD operation? I'm not that familiar with the atomics but 'atomic load' sounds more like a LD...
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.
You're right, my brain farted. Have got this on a local branch.
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.
This has now been pushed
e9b4a6e
to
a2b5eb0
Compare
; CHECK-NEXT: ldd [[RR2:r[0-9]+]], [[RD2:(X|Y|Z)]]+ | ||
; CHECK-NEXT: add [[RR1]], [[TMP:r[0-9]+]] | ||
; CHECK-NEXT: adc [[RR2]], [[TMP:r[0-9]+]] | ||
; CHECK-NEXT: out 63, r0 |
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.
This is starting to look really nice 👍
Don't forget to store the result back to %foo though (see http://llvm.org/docs/LangRef.html#atomicrmw-instruction)
@rjordans did you want to review this? |
Sure no problem, I was mainly trying to follow how this would be implemented and figured that I could just as well give some suggestions whenever I spot things. I hope you didn't mind. Anyway, I guess you missed my last comment but from looking at the LLVM language reference it seems that the atomicrmw operations require a write-back of the new value as well. It seems like you miss that in the currently generated code. Other than that it looks really nice now. |
Suggestions and feedback are always appreciated :) I haven't worked with atomics much either so anything is useful.
I didn't notice that before, nice catch! |
The atomic RMW instructions now store the value back to the original pointer. |
1729811
to
081b3a6
Compare
Have also rebased the branch so it's a bit cleaner. |
This PR adds atomics support to AVR.
Still need to implement
atomic_load_add
atomic_cmp_swap
atomic_swap
atomic_load_sub
atomic_load_and
atomic_load_or
atomic_load_xor
atomic_load_nand
atomic_load_min
atomic_load_max
atomic_load_umin
atomic_load_umax
atomic_fence
Ideally we should use the AVR RMW instructions (
XCH
, etc) when we can. Not a priority though because they aren't well supported.Fixes #197.