[PATCH] D38215: [SelectionDAG] Fix ATOMIC_CMP_SWAP_WITH_SUCCESS expansion

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand created this revision.
Herald added subscribers: kristof.beyls, javed.absar, aemerson.

When expanding a ATOMIC_CMP_SWAP_WITH_SUCCESS, ExpandNode creates a comparison of the compare-and-swap result against the expected value.  If the type is not legal, those values may need to be extended before actually doing the compare.  This code seems incorrect.  The compare-and-swap result is extended correctly, but the expected isn't in all case.

The problem occurs only if TLI.getExtendForAtomicOps returns either ISD::ZERO_EXTEND or ISD::ANY_EXTEND.  In both cases, the current code does:

  RHS = DAG.getNode(ISD::ZERO_EXTEND, dl, OuterType, Node->getOperand(2));

As far as I can tell, this should always be a no-op, since the type of Node->getOperand(2) is already equal OuterType.  This is because the operands were already promoted by LegalizeIntegerTypes before we get there.  But this promotion is just an "any-extend", it doesn't actually perform either zero- or sign-extension.  Since the above line is also a no-op, the extension is then not performed at all.  I have a test case where this causes the comparison to erroneously return failure.

I think ExpandNode should use an in-reg promotion from AtomicType at this point, to ensure that the value is actually extended correctly.  This fixed my test case.

Doing this change causes two ARM test cases to fail since an additional extend instruction is now generated.  In one place, I think this is actually necessary, and the code would have been incorrect without.  In the second place, the extend instruction is redundant since the ARM back-end already generates one in ARMExpandPseudo::ExpandCMP_SWAP, but this is unknown to the SelectionDAG.  This could likely be improved, but the source already says "This only gets used at -O0 so we don't care about efficiency of the generated code.", so this may not be important.


https://reviews.llvm.org/D38215

Files:
  lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
  test/CodeGen/ARM/atomic-cmpxchg.ll
  test/CodeGen/ARM/cmpxchg-O0.ll
  test/CodeGen/SystemZ/cmpxchg-05.ll


Index: test/CodeGen/SystemZ/cmpxchg-05.ll
===================================================================
--- test/CodeGen/SystemZ/cmpxchg-05.ll
+++ test/CodeGen/SystemZ/cmpxchg-05.ll
@@ -79,3 +79,17 @@
   %xres = sext i1 %res to i8
   ret i8 %xres
 }
+
+; Verify that computing the comparison result extends the input value
+; CHECK-LABEL: f7
+; CHECK: llc [[VAL:%r[0-9]+]], 0(%r3)
+; CHECK: llcr [[REG:%r[0-9]+]], [[RES:%r[0-9]+]]
+; CHECK: cr [[REG]], [[VAL]]
+define zeroext i8 @f7(i8* nocapture, i8 *, i8 zeroext) {
+  %val = load i8, i8 *%1
+  %cx = cmpxchg i8* %0, i8 %val, i8 %2 seq_cst seq_cst
+  %res = extractvalue { i8, i1 } %cx, 1
+  %xres = sext i1 %res to i8
+  ret i8 %xres
+}
+
Index: test/CodeGen/ARM/cmpxchg-O0.ll
===================================================================
--- test/CodeGen/ARM/cmpxchg-O0.ll
+++ test/CodeGen/ARM/cmpxchg-O0.ll
@@ -17,7 +17,8 @@
 ; CHECK:     cmp{{(\.w)?}} [[STATUS]], #0
 ; CHECK:     bne [[RETRY]]
 ; CHECK: [[DONE]]:
-; CHECK:     cmp{{(\.w)?}} [[OLD]], [[DESIRED]]
+; CHECK:     uxtb [[TMP:r[0-9]+]], [[DESIRED]]
+; CHECK:     cmp{{(\.w)?}} [[OLD]], [[TMP]]
 ; CHECK:     {{moveq|movweq}} {{r[0-9]+}}, #1
 ; CHECK:     dmb ish
   %res = cmpxchg i8* %addr, i8 %desired, i8 %new seq_cst monotonic
@@ -36,7 +37,8 @@
 ; CHECK:     cmp{{(\.w)?}} [[STATUS]], #0
 ; CHECK:     bne [[RETRY]]
 ; CHECK: [[DONE]]:
-; CHECK:     cmp{{(\.w)?}} [[OLD]], [[DESIRED]]
+; CHECK:     uxth [[TMP:r[0-9]+]], [[DESIRED]]
+; CHECK:     cmp{{(\.w)?}} [[OLD]], [[TMP]]
 ; CHECK:     {{moveq|movweq}} {{r[0-9]+}}, #1
 ; CHECK:     dmb ish
   %res = cmpxchg i16* %addr, i16 %desired, i16 %new seq_cst monotonic
Index: test/CodeGen/ARM/atomic-cmpxchg.ll
===================================================================
--- test/CodeGen/ARM/atomic-cmpxchg.ll
+++ test/CodeGen/ARM/atomic-cmpxchg.ll
@@ -46,9 +46,10 @@
 ; CHECK-ARMV6-NEXT: b [[TRY]]
 
 ; CHECK-THUMBV6-LABEL: test_cmpxchg_res_i8:
-; CHECK-THUMBV6:       mov [[EXPECTED:r[0-9]+]], r1
+; CHECK-THUMBV6:       mov [[TMP:r[0-9]+]], r1
 ; CHECK-THUMBV6-NEXT:  bl __sync_val_compare_and_swap_1
 ; CHECK-THUMBV6-NEXT:  mov [[RES:r[0-9]+]], r0
+; CHECK-THUMBV6-NEXT:  uxtb [[EXPECTED:r[0-9]+]], [[TMP]]
 ; CHECK-THUMBV6-NEXT:  movs r0, #1
 ; CHECK-THUMBV6-NEXT:  movs [[ZERO:r[0-9]+]], #0
 ; CHECK-THUMBV6-NEXT:  cmp [[RES]], [[EXPECTED]]
Index: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
===================================================================
--- lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
+++ lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
@@ -2925,12 +2925,12 @@
     case ISD::ZERO_EXTEND:
       LHS = DAG.getNode(ISD::AssertZext, dl, OuterType, Res,
                         DAG.getValueType(AtomicType));
-      RHS = DAG.getNode(ISD::ZERO_EXTEND, dl, OuterType, Node->getOperand(2));
+      RHS = DAG.getZeroExtendInReg(Node->getOperand(2), dl, AtomicType);
       ExtRes = LHS;
       break;
     case ISD::ANY_EXTEND:
       LHS = DAG.getZeroExtendInReg(Res, dl, AtomicType);
-      RHS = DAG.getNode(ISD::ZERO_EXTEND, dl, OuterType, Node->getOperand(2));
+      RHS = DAG.getZeroExtendInReg(Node->getOperand(2), dl, AtomicType);
       break;
     default:
       llvm_unreachable("Invalid atomic op extension");


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D38215.116467.patch
Type: text/x-patch
Size: 3228 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170925/75eef659/attachment.bin>