[PATCH] [X86] Use the generic AtomicExpandPass instead of X86AtomicExpandPass

JF Bastien jfb at chromium.org
Sat Aug 30 11:21:31 PDT 2014


Were there previously tests for the functionality you're moving? If not, or if they were insufficient, could you augment the tests?

I've never been quite comfortable with synchronization scope, but I just realized that your recent work does away with it. Unconditionally upgrading to cross-thread is OK, but do you think it would be worth keeping the synchronization scope property when it's not cross-thread? I'm not sure how it's actually used (I understand the implications for signal handling, but I'm not sure other parts of the code base are correct). In any case, this should be a separate change.

================
Comment at: lib/CodeGen/AtomicExpandPass.cpp:134
@@ +133,3 @@
+  // atomic swap, that can be implemented for example as a ldrex/strex on ARM
+  // or cmpxchg8/16b on X86, as these are atomic for larger sizes.
+  // It is the responsibility of the target to only return true in
----------------
IIRC they both need a `lock` prefix to be atomic.

================
Comment at: lib/CodeGen/AtomicExpandPass.cpp:261
@@ +260,3 @@
+  AtomicOrdering Order =
+      AI->getOrdering() == Unordered ? Monotonic : AI->getOrdering();
+  AtomicOrdering MemOpOrder =
----------------
I'd rename this to `FenceOrder`.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:16547
@@ +16546,3 @@
+  if (OpWidth == 64)
+    return !Subtarget.is64Bit(); // FIXME this should be Subtarget.hasCmpxchg8b
+  else if (OpWidth == 128)
----------------
Fix the FIXME?

http://reviews.llvm.org/D5090






More information about the llvm-commits mailing list