Page MenuHomePhabricator

[CGAtomic] Lift stronger requirements on cmpxch and add support for acquire failure mode
ClosedPublic

Authored by bruno on Mar 19 2021, 5:31 PM.

Details

Summary
  • Fix emitAtomicCmpXchgFailureSet to support release/acquire (succ/fail) memory order.
  • Remove stronger checks for cmpxch.

Implements http://wg21.link/p0418, related discussions in https://groups.google.com/g/thread-sanitizer/c/bTOVyLAo8p4/m/Y-2yks6ZLAAJ

A follow patch should attempt to implement the TSAN side of failure mode support.

Diff Detail

Event Timeline

bruno created this revision.Mar 19 2021, 5:31 PM
bruno requested review of this revision.Mar 19 2021, 5:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 19 2021, 5:31 PM
jfb accepted this revision.Mar 20 2021, 1:38 PM
jfb added a reviewer: kubamracek.

Maybe refer to http://wg21.link/p0418 directly in the commit message?

This revision is now accepted and ready to land.Mar 20 2021, 1:39 PM
bruno updated this revision to Diff 332823.Mar 23 2021, 4:44 PM
bruno edited the summary of this revision. (Show Details)

@jfb sure!

Also fix test failures.

This revision was landed with ongoing or failed builds.Mar 23 2021, 4:45 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2021, 4:45 PM
yln added a subscriber: yln.Apr 15 2021, 10:25 AM
yln added inline comments.
clang/lib/CodeGen/CGAtomic.cpp
444–447

Should the following assert in AtomicCmpXchgInst::Init() have been changed as well?

assert(!isStrongerThan(FailureOrdering, SuccessOrdering) &&
       "AtomicCmpXchg failure argument shall be no stronger than the success "
       "argument");

https://github.com/llvm/llvm-project/blob/dad5caa59e6b2bde8d6cf5b64a972c393c526c82/llvm/lib/IR/Instructions.cpp#L1561

I am observing a crash on an internal code base most likely caused by this.

yln added inline comments.Apr 19 2021, 11:18 AM
clang/lib/CodeGen/CGAtomic.cpp
444–447

friendly ping @bruno

bruno added a comment.Jun 1 2021, 10:18 AM

@yln sorry, I didn't get notifications for this somehow. This got fixed as part of the more general support in https://github.com/llvm/llvm-project/commit/819e0d105e84c6081cfcfa0e38fd257b6124553a