[selectionDAG][atomics] Add sign extension handling to atom cmp and swap

[sdardis]

Summary

ATOMIC_CMP_AND_SWAP_WITH_SUCCESS needs to generate sign extension nodes for
targets that sign extend their implementation of ATOMIC_CMP_AND_SWAP.

This is required as MIPS canonicalises the result of atomic operations into sign extended
form. The resulting setCC can then have ATOMIC_CMP_AND_SWAP operand which is
signed and an operand which may be unsigned.

[t.p.northover]

Thanks for the generic solution Simon, this looks good to me.

Tim.

[sdardis]

Can you commit on my behalf? Thanks.

[sdardis]

This doesn't fix the 32 bit on 64 bit case.

For 16 on 32bit:

Legalizing: t22: i32,i32,ch = AtomicCmpSwapWithSuccess<Volatile LDST2[%addr]> t11, t2, t4, t6
 ... replacing: t22: i32,i32,ch = AtomicCmpSwapWithSuccess<Volatile LDST2[%addr]> t11, t2, t4, t6
     with:      t27: i32 = AssertSext t25, ValueType:ch:i16
      and:      t30: i32 = setcc t27, t28, seteq:ch
      and:      t25: i32,ch = AtomicCmpSwap<Volatile LDST2[%addr]> t11, t2, t4, t6

Which is what is required. But for 32 on 64bit I'm getting:

Legalizing: t27: i32,i32,ch = AtomicCmpSwapWithSuccess<Volatile LDST4[%addr]> t18, t2, t7, t15
 ... replacing: t27: i32,i32,ch = AtomicCmpSwapWithSuccess<Volatile LDST4[%addr]> t18, t2, t7, t15
     with:      t30: i32,ch = AtomicCmpSwap<Volatile LDST4[%addr]> t18, t2, t7, t15
      and:      t32: i32 = setcc t30, t7, seteq:ch
      and:      t30: i32,ch = AtomicCmpSwap<Volatile LDST4[%addr]> t18, t2, t7, t15

This is after adding:

if (Subtarget.isGP64bit()) {
  AddPromotedToType(ISD::ATOMIC_CMP_SWAP_WITH_SUCCESS, MVT::i32, MVT::i64);
  AddPromotedToType(ISD::ATOMIC_CMP_SWAP, MVT::i32, MVT::i64);
}

To MipsISelLowering.cpp. Any suggestions?

[sdardis]

I was wrong, sorry for the noise.

[t.p.northover]

Oh good, I was just trying to work out what was going wrong. Anyway, it's committed as r264296. Sorry about the delay and thanks for working on it!

Tim.

[sdardis]

Thanks again.

include/llvm/Target/TargetLowering.h

   /// it succeeds, and nullptr otherwise.
   /// If shouldExpandAtomicLoadInIR returns true on that load, it will undergo
   /// another round of expansion.
   virtual LoadInst *
   lowerIdempotentRMWIntoFencedLoad(AtomicRMWInst *RMWI) const {
     return nullptr;
   }
 
+  /// Returns true if the platform's atomic operations are sign extended.
+  virtual bool hasSignExtendedAtomicOps() const {
+    return false;
+  }
+
   /// Returns true if we should normalize
   /// select(N0&N1, X, Y) => select(N0, select(N1, X, Y), Y) and
   /// select(N0|N1, X, Y) => select(N0, select(N1, X, Y, Y)) if it is likely
   /// that it saves us from materializing N0 and N1 in an integer register.
   /// Targets that are able to perform and/or on flags should return false here.
   virtual bool shouldNormalizeToSelectSequence(LLVMContext &Context,
                                                EVT VT) const {
     // If a target has multiple condition registers, then it likely has logical

lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

case ISD::ATOMIC_CMP_SWAP_WITH_SUCCESS: {
     SDValue Res = DAG.getAtomicCmpSwap(
         ISD::ATOMIC_CMP_SWAP, dl, cast<AtomicSDNode>(Node)->getMemoryVT(), VTs,
         Node->getOperand(0), Node->getOperand(1), Node->getOperand(2),
         Node->getOperand(3), cast<MemSDNode>(Node)->getMemOperand(),
         cast<AtomicSDNode>(Node)->getSuccessOrdering(),
         cast<AtomicSDNode>(Node)->getFailureOrdering(),
         cast<AtomicSDNode>(Node)->getSynchScope());
 
-    SDValue Success = DAG.getSetCC(SDLoc(Node), Node->getValueType(1),
-                                   Res, Node->getOperand(2), ISD::SETEQ);
+    SDValue LHS = Res;
+    SDValue RHS = Node->getOperand(1);
+
+    EVT AtomicType = cast<AtomicSDNode>(Node)->getMemoryVT();
+    EVT OuterType = Node->getValueType(0);
+    if (TLI.hasSignExtendedAtomicOps()) {
+      LHS = DAG.getNode(ISD::AssertSext, dl, OuterType, Res,
+                        DAG.getValueType(AtomicType));
+      RHS = DAG.getNode(ISD::SIGN_EXTEND_INREG, dl, OuterType,
+                        Node->getOperand(2), DAG.getValueType(AtomicType));
+    } else {
+      LHS = DAG.getNode(ISD::AssertZext, dl, OuterType, Res, DAG.getValueType(AtomicType));
+      RHS = DAG.getNode(ISD::ZERO_EXTEND, dl, OuterType, Node->getOperand(2));
+    }
+
+    SDValue Success =
+        DAG.getSetCC(dl, Node->getValueType(1), LHS, RHS, ISD::SETEQ);
 
-    Results.push_back(Res.getValue(0));
+    Results.push_back(LHS.getValue(0));
     Results.push_back(Success);
     Results.push_back(Res.getValue(1));
     break;
   }
   case ISD::DYNAMIC_STACKALLOC:
     ExpandDYNAMIC_STACKALLOC(Node, Results);
     break;
   case ISD::MERGE_VALUES:

lib/Target/Mips/MipsISelLowering.h

public:
 
     MVT getScalarShiftAmountTy(const DataLayout &, EVT) const override {
       return MVT::i32;
     }
 
     bool isCheapToSpeculateCttz() const override;
     bool isCheapToSpeculateCtlz() const override;
 
+    bool hasSignExtendedAtomicOps() const override {
+      return true;
+    }
+
     void LowerOperationWrapper(SDNode *N,
                                SmallVectorImpl<SDValue> &Results,
                                SelectionDAG &DAG) const override;
 
     /// LowerOperation - Provide custom lowering hooks for some operations.
     SDValue LowerOperation(SDValue Op, SelectionDAG &DAG) const override;
 
     /// ReplaceNodeResults - Replace the results of node with an illegal result

test/CodeGen/Mips/atomic.ll

 ; MIPSR6:        beqzc   $[[R16]], $[[BB0]]
 
 ; ALL:       $[[BB1]]:
 ; ALL:           srlv    $[[R17:[0-9]+]], $[[R14]], $[[R5]]
 
 ; NO-SEB-SEH:    sll     $[[R18:[0-9]+]], $[[R17]], 24
 ; NO-SEB-SEH:    sra     $[[R19:[0-9]+]], $[[R18]], 24
 
+; FIXME: -march=mips produces a redundant sign extension here...
+; NO-SEB-SEH:    sll     $[[R20:[0-9]+]], $5, 24
+; NO-SEB-SEH:    sra     $[[R20]], $[[R20]], 24
+
 ; HAS-SEB-SEH:   seb     $[[R19:[0-9]+]], $[[R17]]
 
-; ALL:           xor     $[[R20:[0-9]+]], $[[R19]], $5
-; ALL:           sltiu   $2, $[[R20]], 1
+; FIXME: ...Leading to this split check.
+; NO-SEB-SEH:    xor     $[[R21:[0-9]+]], $[[R19]], $[[R20]]
+; HAS-SEB-SEH:   xor     $[[R21:[0-9]+]], $[[R19]], $5
+
+; ALL: sltiu   $2, $[[R21]], 1
 }
 
 ; Check one i16 so that we cover the seh sign extend
 @z = common global i16 0, align 1
 
 define signext i16 @AtomicLoadAdd16(i16 signext %incr) nounwind {
 entry:
   %0 = atomicrmw add i16* @z, i16 %incr monotonic

 ; ALL:           srlv    $[[R16:[0-9]+]], $[[R15]], $[[R5]]
 
 ; NO-SEB-SEH:    sll     $[[R17:[0-9]+]], $[[R16]], 16
 ; NO-SEB-SEH:    sra     $2, $[[R17]], 16
 
 ; MIPS32R2:      seh     $2, $[[R16]]
 }
 
+; Test that the i16 return value from cmpxchg is recognised as signed,
+; so that setCC doesn't end up comparing an unsigned value to a signed
+; value.
+; The rest of the functions here are testing the atomic expansion, so
+; we just match the end of the function.
+define {i16, i1} @foo(i16* %addr, i16 %l, i16 %r, i16 %new) {
+  %desired = add i16 %l, %r
+  %res = cmpxchg i16* %addr, i16 %desired, i16 %new seq_cst seq_cst
+  ret {i16, i1} %res
+
+; ALL-LABEL: foo
+; MIPSR6:        addu    $[[R2:[0-9]+]], $[[R1:[0-9]+]], $[[R0:[0-9]+]]
+; NOT-MICROMIPS: addu    $[[R2:[0-9]+]], $[[R1:[0-9]+]], $[[R0:[0-9]+]]
+; MICROMIPS:     addu16  $[[R2:[0-9]+]], $[[R1:[0-9]+]], $[[R0:[0-9]+]]
+
+; ALL:           sync
+
+; ALL:           andi    $[[R3:[0-9]+]], $[[R2]], 65535
+; ALL:       $[[BB0:[A-Z_0-9]+]]:
+; ALL:           ll      $[[R4:[0-9]+]], 0($[[R5:[0-9]+]])
+; ALL:           and     $[[R6:[0-9]+]], $[[R4]], $
+; ALL:           and     $[[R7:[0-9]+]], $[[R4]], $
+; ALL:           or      $[[R8:[0-9]+]], $[[R7]], $
+; ALL:           sc      $[[R8]], 0($[[R5]])
+; NOT-MICROMIPS: beqz    $[[R8]], $[[BB0]]
+; MICROMIPS:     beqzc   $[[R8]], $[[BB0]]
+; MIPSR6:        beqzc   $[[R8]], $[[BB0]]
+
+; ALL:           srlv    $[[R9:[0-9]+]], $[[R6]], $
+
+; NO-SEB-SEH:    sll     $[[R10:[0-9]+]], $[[R9]], 16
+; NO-SEB-SEH:    sra     $[[R11:[0-9]+]], $[[R10]], 16
+
+; NO-SEB-SEH:    sll     $[[R12:[0-9]+]], $[[R2]], 16
+; NO-SEB-SEH:    sra     $[[R13:[0-9]+]], $[[R12]], 16
+
+; HAS-SEB-SEH:   seh     $[[R11:[0-9]+]], $[[R9]]
+; HAS-SEB-SEH:   seh     $[[R13:[0-9]+]], $[[R2]]
+
+; ALL:           xor     $[[R12:[0-9]+]], $[[R11]], $[[R13]]
+; ALL:           sltiu   $3, $[[R12]], 1
+; ALL:           sync
+}
 
 @countsint = common global i32 0, align 4
 
 define i32 @CheckSync(i32 signext %v) nounwind noinline {
 entry:
   %0 = atomicrmw add i32* @countsint, i32 %v seq_cst
   ret i32 %0