[PATCH] IR support for "cmpxchg weak"

[Chandler Carruth]

Lots of little nits, nothing substantial. LGTM, happy for you to commit in the granularity you want with as many of the fixes as seem good, none are essential or really necessary prior to it going in...

================
Comment at: docs/Atomics.rst:432-433
@@ -432,4 +431,4 @@
 
-On ARM, MIPS, and many other RISC architectures, Acquire, Release, and
-SequentiallyConsistent semantics require barrier instructions for every such
+On ARM (before v8), MIPS, and many other RISC architectures, Acquire, Release,
+and SequentiallyConsistent semantics require barrier instructions for every such
 operation. Loads and stores generate normal instructions.  ``cmpxchg`` and
----------------
This is an unrelated cleanup, you could submit separately if you want.... but maybe not worth splitting patches out.

================
Comment at: docs/LangRef.rst:5086-5089
@@ -5085,6 +5085,6 @@
 The success and failure :ref:`ordering <ordering>` arguments specify how this
-``cmpxchg`` synchronizes with other atomic operations. The both ordering
-parameters must be at least ``monotonic``, the ordering constraint on failure
-must be no stronger than that on success, and the failure ordering cannot be
-either ``release`` or ``acq_rel``.
+``cmpxchg`` synchronizes with other atomic operations. Both ordering parameters
+must be at least ``monotonic``, the ordering constraint on failure must be no
+stronger than that on success, and the failure ordering cannot be either
+``release`` or ``acq_rel``.
 
----------------
This one too.

================
Comment at: docs/LangRef.rst:5107-5110
@@ -5106,1 +5106,6 @@
+
+If the cmpxchg operation is marked as ``weak`` then a spurious failure is
+permitted: the operation may not write ``<new>`` even if the comparison
+matched. Otherwise, the i1 value is 1 if and only if the value loaded
+equals ``cmp``.
 
----------------
The "Otherwise" here is hard for me to read because of the preceding clause. Maybe just re-state the inverse predicate?

================
Comment at: docs/LangRef.rst:5122
@@ -5116,3 +5121,3 @@
     entry:
-      %orig = atomic load i32* %ptr unordered                   ; yields {i32}
+      %orig = atomic load i32* %ptr unordered                   ; yields i32
       br label %loop
----------------
I really wonder why there were {i32}s throughout this.... Those could also go in a separate cleanup patch. At this point, that might be worthwhile, I see numerous other fixes of {i32} below for atomicrmw.

================
Comment at: lib/AsmParser/LLParser.cpp:4259
@@ -4258,3 +4258,3 @@
 /// ParseCmpXchg
-///   ::= 'cmpxchg' 'volatile'? TypeAndValue ',' TypeAndValue ',' TypeAndValue
-///       'singlethread'? AtomicOrdering AtomicOrdering
+///   ::= 'cmpxchg' weak? 'volatile'? TypeAndValue ',' TypeAndValue ','
+///       TypeAndValue 'singlethread'? AtomicOrdering AtomicOrdering
----------------
'weak'?

================
Comment at: lib/AsmParser/LLParser.cpp:4270
@@ +4269,3 @@
+
+  if (EatIfPresent(lltok::kw_weak))
+    isWeak = true;
----------------
oh my, we already had the keyword from linkage...

should we update any of the organization of these in the LLLexer / LLToken code to make it less confusing that this keyword is used both as an instruction flag and a linkage spec keyword? anything else work this way already?

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:2958
@@ +2957,3 @@
+        I = ExtractValueInst::Create(I, 0);
+      } else
+        cast<AtomicCmpXchgInst>(I)->setWeak(Record[OpNum+4]);
----------------
matching {}s on both sides of the else?

================
Comment at: lib/CodeGen/AtomicExpandLoadLinkedPass.cpp:325-326
@@ -333,2 +324,4 @@
 
-  CI->replaceAllUsesWith(Loaded);
+  std::for_each(PrunedInsts.begin(), PrunedInsts.end(),
+                [](ExtractValueInst *EV) { EV->eraseFromParent(); });
+
----------------
Range based loop?

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:142
@@ -142,1 +141,3 @@
+  case ISD::ATOMIC_CMP_SWAP_WITH_SUCCESS:
+    Res = PromoteIntRes_AtomicCmpSwap(cast<AtomicSDNode>(N), ResNo);break;
   }
----------------
Weird spacing here, just put break on its own line?

================
Comment at: lib/Transforms/Instrumentation/ThreadSanitizer.cpp:535-539
@@ -534,3 +534,7 @@
                      createOrdering(&IRB, CASI->getFailureOrdering())};
-    CallInst *C = CallInst::Create(TsanAtomicCAS[Idx], ArrayRef<Value*>(Args));
-    ReplaceInstWithInst(I, C);
+    CallInst *C = IRB.CreateCall(TsanAtomicCAS[Idx], Args);
+    Value *Success = IRB.CreateICmpEQ(C, CASI->getCompareOperand());
+
+    Value *Res = IRB.CreateInsertValue(UndefValue::get(CASI->getType()), C, 0);
+    Res = IRB.CreateInsertValue(Res, Success, 1);
+
----------------
Might want to talk with TSan folks about changing the interface in similar ways to remove the icmp....

================
Comment at: test/CodeGen/PowerPC/Atomics-32.ll:532-533
@@ -531,3 +531,4 @@
   %1 = load i8* @sc, align 1
-  %2 = cmpxchg i8* @sc, i8 %0, i8 %1 monotonic monotonic
+  %pair2 = cmpxchg i8* @sc, i8 %0, i8 %1 monotonic monotonic
+  %2 = extractvalue { i8, i1 } %pair2, 0
   store i8 %2, i8* @sc, align 1
----------------
your clever naming here entertains me. =]

http://reviews.llvm.org/D4134