This is an archive of the discontinued LLVM Phabricator instance.

[mips][compiler-rt] Provide 64bit atomic add and sub
ClosedPublic

Authored by sdardis on Nov 23 2017, 2:11 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

sdardis created this revision.Nov 23 2017, 2:11 AM

I'll defer to @kcc or @eugenis here on how to handle these potentially better in sanitizer_atomic_clang.h.

dberris accepted this revision.Nov 23 2017, 5:03 AM

From the XRay perspective, this seems fine to me.

This revision is now accepted and ready to land.Nov 23 2017, 5:03 AM
dberris added inline comments.Nov 23 2017, 5:20 AM
lib/sanitizer_common/sanitizer_atomic_clang.h
53 ↗(On Diff #124052)

Looking at this, why not just sizeof(T)?

69 ↗(On Diff #124052)

Same here.

sdardis added inline comments.Nov 23 2017, 5:27 AM
lib/sanitizer_common/sanitizer_atomic_clang.h
53 ↗(On Diff #124052)

When I saw the linking error, I was reminded of D31803, so I cribbed the implementation of this change from atomic_compare_exchange_strong below. I adjust shortly.

sdardis updated this revision to Diff 124216.Nov 24 2017, 8:51 AM

Update as per initial review comments.

kcc added a comment.Dec 1 2017, 3:40 PM

This change introduces more ifdefs to this file, which is bad.
It already has one ifdef -- and that one is already for "defined(_MIPS_SIM)"
Please don't introduce any more ifdefs, instead please add a new file (something_something_mips.h) and remove the only ifdef from here.

sdardis updated this revision to Diff 125741.Dec 6 2017, 8:48 AM

Review comments addressed.

atanasyan accepted this revision.Dec 11 2017, 2:59 AM

LGTM if you run clang-format on sanitizer_atomic_clang_mips.h.

dberris accepted this revision.Dec 11 2017, 3:00 AM
sdardis updated this revision to Diff 126334.Dec 11 2017, 3:50 AM

Address review comments.

Herald added a subscriber: Restricted Project. · View Herald TranscriptDec 11 2017, 3:50 AM
This revision was automatically updated to reflect the committed changes.

Thanks for the review.