[PATCH] [X86] Use the generic AtomicExpandPass instead of X86AtomicExpandPass
jfb at chromium.org
Wed Sep 3 13:46:08 PDT 2014
>>! In D5090#10, @morisset wrote:
> There was already a lot of tests for atomics on X86 (test/CodeGen/X86/atomic*), so I did not modify them in this patch, just checked that they still pass.
> I agree that preserving the synchronization scope might be good, but that would be a different patch (the code in X86AtomicAtomicPass does not look like it was preserving synchronization scope either).
Comment at: lib/CodeGen/AtomicExpandPass.cpp:298
@@ +297,3 @@
+ LoadInst *InitLoaded = Builder.CreateLoad(Addr);
> jfb wrote:
> > Could you also explain the rationale behind the alignment (C11/C++11 memory model requires at least natural alignment, and the IR has the same guarantee).
> All of this code was just moved from the X86AtomicExpandPass, I am not entirely sure of why it does things in a specific way.
> But is it not providing exactly this natural alignment that you mention ?
Yes, but to an unfamiliar reader it's not obvious why this alignment is the right one, so I think a comment that says "same as C++11 guarantees" would be helpful.
Comment at: lib/Target/X86/X86ISelLowering.cpp:16615
@@ -16614,1 +16614,3 @@
+/// Returns true if the operand type is 1 step up from the native width, and
+/// the corresponding cmpxchg8b or cmpxchg16b instruction is available.
"1 step up" is confusing, I'd just say "larger than the native atomic store width".
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)
> jfb wrote:
> > Fix the FIXME?
> This code was moved from X86AtomicExpandPass. Because fixing this FIXME would not be entirely trivial (need to add hasCmpxchg8b to the subtarget, find which ones offer it, and so on), and is orthogonal to this change, I chose not to tackle it in this patch.
More information about the llvm-commits