[mips] Remove unnecessary sign extension in atomics

[sdardis]

Summary

emitAtomic*PartWord unconditionally sign extends its result causing failures
with __c11_atomic_compare_exchange_strong & co due to an unsigned value
being compared to a sign extended value.

[dsanders]

Hi,

It's my understanding that this patch changes AtomicCmpSwap nodes to produce zero-extended results instead so that it compares properly against an 'i32 = AssertZext ..., ValueType:ch:i16'. What happens when it's an AssertSext instead? I think we're moving the problem from one case to the other.

I think the bug may be in the target-independent legalization passes rather than in our backend. Our initial selection DAG (with the irrelevant bits removed) is:

t6: i32 = AssertZext t2, ValueType:ch:i16
t7: i16 = truncate t6
t11: i16,i1,ch = AtomicCmpSwapWithSuccess<Volatile LDST2[@z]> t0, GlobalAddress:i32<i16* @z> 0, t7, t9
t12: i32 = zero_extend t11:1

After type-legalization it becomes:

t6: i32 = AssertZext t2, ValueType:ch:i16
t17: i32,i32,ch = AtomicCmpSwapWithSuccess<Volatile LDST2[@z]> t0, GlobalAddress:i32<i16* @z> 0, t6, t8
t19: i32 = and t17:1, Constant:i32<1>

which is fine so long as the comparison inside AtomicCmpSwapWithSuccess is performed as if it's an i16 comparison and is not affected by bits 16-31. Then after operation-legalization we have:

t6: i32 = AssertZext t2, ValueType:ch:i16
t20: i32,ch = AtomicCmpSwap<Volatile LDST2[@z]> t0, t27, t6, t8
t22: i32 = setcc t20, t6, seteq:ch

I think this step introduced the bug. Whereas the type-legalized code had an i16 comparison we now have an i32-comparison that tries to compare the zero-extended t6 against the sign-extended t20. I think SelectionDAGLegalize::ExpandNode() needs to inject a sign/zero extension into the operands of the SETCC it emits like so:

t6: i32 = AssertZext t2, ValueType:ch:i16
t20: i32,ch = AtomicCmpSwap<Volatile LDST2[@z]> t0, t27, t6, t8
t40: i32 = zero_extend_inreg t6
t41: i32 = zero_extend_inreg t20
t22: i32 = setcc t41, t40, seteq:ch

Assuming this is correct so far, there's two things I'm not sure of:

1. Is AtomicCmpSwap's result defined to be sign/zero/any extended? It's probably any-extended but I don't see any documentation saying this.
2. How do other targets avoid this bug? Are they just lucky or is there something they know that we don't?

test/CodeGen/Mips/atomic.ll

350

Shouldn't we consume the i1 result? It's my understanding that the comparison used to produce the success result is comparing the sign-extended result of the AtomicCmpSwap and with the zero-extended cmp operand. The inconsistency in the extensions causes our GPR-width comparisons to give incorrect results.

[sdardis]

Abandoned in favor of http://reviews.llvm.org/D18241 . We have to keep the sign extension for consistency with 32bit on 64bit, as ll sign extends its result. Other targets appear to be zero extending their results, so any existing bug there is probably latent.

lib/Target/Mips/MipsISelLowering.cpp

MipsTargetLowering::emitAtomicBinary(MachineInstr *MI, MachineBasicBlock *BB,
   BuildMI(BB, DL, TII->get(SC), Success).addReg(StoreVal).addReg(Ptr).addImm(0);
   BuildMI(BB, DL, TII->get(BEQ)).addReg(Success).addReg(ZERO).addMBB(loopMBB);
 
   MI->eraseFromParent(); // The instruction is gone now.
 
   return exitMBB;
 }
 
-MachineBasicBlock *MipsTargetLowering::emitSignExtendToI32InReg(
-    MachineInstr *MI, MachineBasicBlock *BB, unsigned Size, unsigned DstReg,
-    unsigned SrcReg) const {
-  const TargetInstrInfo *TII = Subtarget.getInstrInfo();
-  DebugLoc DL = MI->getDebugLoc();
-
-  if (Subtarget.hasMips32r2() && Size == 1) {
-    BuildMI(BB, DL, TII->get(Mips::SEB), DstReg).addReg(SrcReg);
-    return BB;
-  }
-
-  if (Subtarget.hasMips32r2() && Size == 2) {
-    BuildMI(BB, DL, TII->get(Mips::SEH), DstReg).addReg(SrcReg);
-    return BB;
-  }
-
-  MachineFunction *MF = BB->getParent();
-  MachineRegisterInfo &RegInfo = MF->getRegInfo();
-  const TargetRegisterClass *RC = getRegClassFor(MVT::i32);
-  unsigned ScrReg = RegInfo.createVirtualRegister(RC);
-
-  assert(Size < 32);
-  int64_t ShiftImm = 32 - (Size * 8);
-
-  BuildMI(BB, DL, TII->get(Mips::SLL), ScrReg).addReg(SrcReg).addImm(ShiftImm);
-  BuildMI(BB, DL, TII->get(Mips::SRA), DstReg).addReg(ScrReg).addImm(ShiftImm);
-
-  return BB;
-}
-
 MachineBasicBlock *MipsTargetLowering::emitAtomicBinaryPartword(
     MachineInstr *MI, MachineBasicBlock *BB, unsigned Size, unsigned BinOpcode,
     bool Nand) const {
   assert((Size == 1 || Size == 2) &&
          "Unsupported size for EmitAtomicBinaryPartial.");
 
   MachineFunction *MF = BB->getParent();
   MachineRegisterInfo &RegInfo = MF->getRegInfo();
MachineBasicBlock *MipsTargetLowering::emitAtomicBinaryPartword(
   unsigned MaskLSB2 = RegInfo.createVirtualRegister(RC);
   unsigned PtrLSB2 = RegInfo.createVirtualRegister(RC);
   unsigned MaskUpper = RegInfo.createVirtualRegister(RC);
   unsigned AndRes = RegInfo.createVirtualRegister(RC);
   unsigned BinOpRes = RegInfo.createVirtualRegister(RC);
   unsigned MaskedOldVal0 = RegInfo.createVirtualRegister(RC);
   unsigned StoreVal = RegInfo.createVirtualRegister(RC);
   unsigned MaskedOldVal1 = RegInfo.createVirtualRegister(RC);
-  unsigned SrlRes = RegInfo.createVirtualRegister(RC);
   unsigned Success = RegInfo.createVirtualRegister(RC);
 
   // insert new blocks after the current block
   const BasicBlock *LLVM_BB = BB->getBasicBlock();
   MachineBasicBlock *loopMBB = MF->CreateMachineBasicBlock(LLVM_BB);
   MachineBasicBlock *sinkMBB = MF->CreateMachineBasicBlock(LLVM_BB);
   MachineBasicBlock *exitMBB = MF->CreateMachineBasicBlock(LLVM_BB);
   MachineFunction::iterator It = ++BB->getIterator();
MachineBasicBlock *MipsTargetLowering::emitAtomicBinaryPartword(
   unsigned SC = isMicroMips ? Mips::SC_MM : Mips::SC;
   BuildMI(BB, DL, TII->get(SC), Success)
     .addReg(StoreVal).addReg(AlignedAddr).addImm(0);
   BuildMI(BB, DL, TII->get(Mips::BEQ))
     .addReg(Success).addReg(Mips::ZERO).addMBB(loopMBB);
 
   //  sinkMBB:
   //    and     maskedoldval1,oldval,mask
-  //    srl     srlres,maskedoldval1,shiftamt
-  //    sign_extend dest,srlres
+  //    srl     dest,maskedoldval1,shiftamt
   BB = sinkMBB;
 
   BuildMI(BB, DL, TII->get(Mips::AND), MaskedOldVal1)
     .addReg(OldVal).addReg(Mask);
-  BuildMI(BB, DL, TII->get(Mips::SRLV), SrlRes)
+  BuildMI(BB, DL, TII->get(Mips::SRLV), Dest)
       .addReg(MaskedOldVal1).addReg(ShiftAmt);
-  BB = emitSignExtendToI32InReg(MI, BB, Size, Dest, SrlRes);
 
   MI->eraseFromParent(); // The instruction is gone now.
 
   return exitMBB;
 }
 
 MachineBasicBlock * MipsTargetLowering::emitAtomicCmpSwap(MachineInstr *MI,
                                                           MachineBasicBlock *BB,
MipsTargetLowering::emitAtomicCmpSwapPartword(MachineInstr *MI,
   unsigned ShiftedNewVal = RegInfo.createVirtualRegister(RC);
   unsigned MaskLSB2 = RegInfo.createVirtualRegister(RC);
   unsigned PtrLSB2 = RegInfo.createVirtualRegister(RC);
   unsigned MaskUpper = RegInfo.createVirtualRegister(RC);
   unsigned MaskedCmpVal = RegInfo.createVirtualRegister(RC);
   unsigned MaskedNewVal = RegInfo.createVirtualRegister(RC);
   unsigned MaskedOldVal1 = RegInfo.createVirtualRegister(RC);
   unsigned StoreVal = RegInfo.createVirtualRegister(RC);
-  unsigned SrlRes = RegInfo.createVirtualRegister(RC);
   unsigned Success = RegInfo.createVirtualRegister(RC);
 
   // insert new blocks after the current block
   const BasicBlock *LLVM_BB = BB->getBasicBlock();
   MachineBasicBlock *loop1MBB = MF->CreateMachineBasicBlock(LLVM_BB);
   MachineBasicBlock *loop2MBB = MF->CreateMachineBasicBlock(LLVM_BB);
   MachineBasicBlock *sinkMBB = MF->CreateMachineBasicBlock(LLVM_BB);
   MachineBasicBlock *exitMBB = MF->CreateMachineBasicBlock(LLVM_BB);
BuildMI(BB, DL, TII->get(Mips::OR), StoreVal)
     .addReg(MaskedOldVal1).addReg(ShiftedNewVal);
   unsigned SC = isMicroMips ? Mips::SC_MM : Mips::SC;
   BuildMI(BB, DL, TII->get(SC), Success)
       .addReg(StoreVal).addReg(AlignedAddr).addImm(0);
   BuildMI(BB, DL, TII->get(Mips::BEQ))
       .addReg(Success).addReg(Mips::ZERO).addMBB(loop1MBB);
 
   //  sinkMBB:
-  //    srl     srlres,maskedoldval0,shiftamt
-  //    sign_extend dest,srlres
+  //    srl     dest,maskedoldval0,shiftamt
   BB = sinkMBB;
 
-  BuildMI(BB, DL, TII->get(Mips::SRLV), SrlRes)
+  BuildMI(BB, DL, TII->get(Mips::SRLV), Dest)
       .addReg(MaskedOldVal0).addReg(ShiftAmt);
-  BB = emitSignExtendToI32InReg(MI, BB, Size, Dest, SrlRes);
 
   MI->eraseFromParent();   // The instruction is gone now.
 
   return exitMBB;
 }
 
 MachineBasicBlock *MipsTargetLowering::emitSEL_D(MachineInstr *MI,
                                                  MachineBasicBlock *BB) const {

test/CodeGen/Mips/atomic.ll

 ; ALL:           srlv    $[[R16:[0-9]+]], $[[R15]], $[[R5]]
 
 ; NO-SEB-SEH:    sll     $[[R17:[0-9]+]], $[[R16]], 24
 ; NO-SEB-SEH:    sra     $2, $[[R17]], 24
 
 ; HAS-SEB-SEH:   seb     $2, $[[R16]]
 }
 
+define zeroext i16 @AtomicLoadNand16Unsigned(i16 zeroext %incr) nounwind {
+entry:
+  %0 = atomicrmw nand i16* @z, i16 %incr monotonic
+  ret i16 %0
+
+; ALL-LABEL: AtomicLoadNand16Unsigned:
+
+; MIPS32-ANY:    lw      $[[R0:[0-9]+]], %got(z)
+; MIPS64-ANY:    ld      $[[R0:[0-9]+]], %got_disp(z)(
+
+; ALL:           addiu   $[[R1:[0-9]+]], $zero, -4
+; ALL:           and     $[[R2:[0-9]+]], $[[R0]], $[[R1]]
+; ALL:           andi    $[[R3:[0-9]+]], $[[R0]], 3
+; CHECK-EL:      sll     $[[R5:[0-9]+]], $[[R3]], 3
+; CHECK-EB:      xori    $[[R4:[0-9]+]], $[[R3]], 2
+; CHECK-EB:      sll     $[[R5:[0-9]+]], $[[R4]], 3
+; ALL:           ori     $[[R6:[0-9]+]], $zero, 65535
+; ALL:           sllv    $[[R7:[0-9]+]], $[[R6]], $[[R5]]
+; ALL:           nor     $[[R8:[0-9]+]], $zero, $[[R7]]
+; ALL:           sllv    $[[R9:[0-9]+]], $4, $[[R5]]
+
+; ALL:       $[[BB0:[A-Z_0-9]+]]:
+; ALL:           ll      $[[R10:[0-9]+]], 0($[[R2]])
+; ALL:           and     $[[R18:[0-9]+]], $[[R10]], $[[R9]]
+; ALL:           nor     $[[R11:[0-9]+]], $zero, $[[R18]]
+; ALL:           and     $[[R12:[0-9]+]], $[[R11]], $[[R7]]
+; ALL:           and     $[[R13:[0-9]+]], $[[R10]], $[[R8]]
+; ALL:           or      $[[R14:[0-9]+]], $[[R13]], $[[R12]]
+; ALL:           sc      $[[R14]], 0($[[R2]])
+; NOT-MICROMIPS: beqz    $[[R14]], $[[BB0]]
+; MICROMIPS:     beqzc   $[[R14]], $[[BB0]]
+
+; ALL:           and     $[[R15:[0-9]+]], $[[R10]], $[[R7]]
+; ALL:           srlv    $[[R16:[0-9]+]], $[[R15]], $[[R5]]
+
+; ALL-NOT:    sll     $[[R17:[0-9]+]], $[[R16]], 16
+; ALL-NOT:    sra     $2, $[[R17]], 16
+
+; ALL-NOT:   seb     $2, $[[R16]]
+}
+
 define signext i8 @AtomicSwap8(i8 signext %newval) nounwind {
 entry:
   %0 = atomicrmw xchg i8* @y, i8 %newval monotonic
   ret i8 %0
 
 ; ALL-LABEL: AtomicSwap8:
 
 ; MIPS32-ANY:    lw      $[[R0:[0-9]+]], %got(y)

 ; ALL:           srlv    $[[R17:[0-9]+]], $[[R14]], $[[R5]]
 
 ; NO-SEB-SEH:    sll     $[[R18:[0-9]+]], $[[R17]], 24
 ; NO-SEB-SEH:    sra     $2, $[[R18]], 24
 
 ; HAS-SEB-SEH:   seb     $2, $[[R17]]
 }
 
+define zeroext i8 @AtomicCmpSwap8Unsigned(i8 zeroext %oldval, i8 zeroext %newval) nounwind {
+entry:
+  %pair0 = cmpxchg i8* @y, i8 %oldval, i8 %newval monotonic monotonic
+  %0 = extractvalue { i8, i1 } %pair0, 0
+  ret i8 %0
+
+; ALL-LABEL: AtomicCmpSwap8Unsigned:
+
+; MIPS32-ANY:    lw      $[[R0:[0-9]+]], %got(y)
+; MIPS64-ANY:    ld      $[[R0:[0-9]+]], %got_disp(y)(
+
+; ALL:           addiu   $[[R1:[0-9]+]], $zero, -4
+; ALL:           and     $[[R2:[0-9]+]], $[[R0]], $[[R1]]
+; ALL:           andi    $[[R3:[0-9]+]], $[[R0]], 3
+; CHECK-EL:      sll     $[[R5:[0-9]+]], $[[R3]], 3
+; CHECK-EB:      xori    $[[R4:[0-9]+]], $[[R3]], 3
+; CHECK-EB:      sll     $[[R5:[0-9]+]], $[[R4]], 3
+; ALL:           ori     $[[R6:[0-9]+]], $zero, 255
+; ALL:           sllv    $[[R7:[0-9]+]], $[[R6]], $[[R5]]
+; ALL:           nor     $[[R8:[0-9]+]], $zero, $[[R7]]
+; ALL:           andi    $[[R9:[0-9]+]], $4, 255
+; ALL:           sllv    $[[R10:[0-9]+]], $[[R9]], $[[R5]]
+; ALL:           andi    $[[R11:[0-9]+]], $5, 255
+; ALL:           sllv    $[[R12:[0-9]+]], $[[R11]], $[[R5]]
+
+; ALL:       $[[BB0:[A-Z_0-9]+]]:
+; ALL:           ll      $[[R13:[0-9]+]], 0($[[R2]])
+; ALL:           and     $[[R14:[0-9]+]], $[[R13]], $[[R7]]
+; ALL:           bne     $[[R14]], $[[R10]], $[[BB1:[A-Z_0-9]+]]
+
+; ALL:           and     $[[R15:[0-9]+]], $[[R13]], $[[R8]]
+; ALL:           or      $[[R16:[0-9]+]], $[[R15]], $[[R12]]
+; ALL:           sc      $[[R16]], 0($[[R2]])
+; NOT-MICROMIPS: beqz    $[[R16]], $[[BB0]]
+; MICROMIPS:     beqzc   $[[R16]], $[[BB0]]
+
+; ALL:       $[[BB1]]:
+; ALL:           srlv    $[[R17:[0-9]+]], $[[R14]], $[[R5]]
+
+; ALL-NOT:    sll     $[[R18:[0-9]+]], $[[R17]], 24
+; ALL-NOT:    sra     $2, $[[R18]], 24
+
+; ALL-NOT:   seb     $2, $[[R17]]
+}
+
 define i1 @AtomicCmpSwapRes8(i8* %ptr, i8 signext %oldval, i8 signext %newval) nounwind {
 entry:
   %0 = cmpxchg i8* %ptr, i8 %oldval, i8 %newval monotonic monotonic
   %1 = extractvalue { i8, i1 } %0, 1
   ret i1 %1
 ; ALL-LABEL: AtomicCmpSwapRes8
 
 ; ALL:           addiu   $[[R1:[0-9]+]], $zero, -4

 ; ALL:           or      $[[R16:[0-9]+]], $[[R15]], $[[R12]]
 ; ALL:           sc      $[[R16]], 0($[[R2]])
 ; NOT-MICROMIPS: beqz    $[[R16]], $[[BB0]]
 ; MICROMIPS:     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
-
-; HAS-SEB-SEH:   seb     $[[R19:[0-9]+]], $[[R17]]
-
-; ALL:           xor     $[[R20:[0-9]+]], $[[R19]], $5
-; ALL:           sltiu   $2, $[[R20]], 1
+; ALL:           xor     $[[R18:[0-9]+]], $[[R17]], $5
+; ALL:           sltiu   $2, $[[R18]], 1
 }
 
-; Check one i16 so that we cover the seh sign extend
 @z = common global i16 0, align 1
+; Check one unsigned i16 so we cover the zero extend case
+define zeroext i16 @AtomicCmpSwap16Unsigned(i16 zeroext %oldval, i16 zeroext %newval) nounwind {
+entry:
+  %pair0 = cmpxchg i16* @z, i16 %oldval, i16 %newval monotonic monotonic
+  %0 = extractvalue { i16, i1 } %pair0, 0
+  ret i16 %0
+
+; ALL-LABEL: AtomicCmpSwap16Unsigned:
+
+; MIPS32-ANY:    lw      $[[R0:[0-9]+]], %got(z)
+; MIPS64-ANY:    ld      $[[R0:[0-9]+]], %got_disp(z)(
+
+; ALL:           addiu   $[[R1:[0-9]+]], $zero, -4
+; ALL:           and     $[[R2:[0-9]+]], $[[R0]], $[[R1]]
+; ALL:           andi    $[[R3:[0-9]+]], $[[R0]], 3
+; CHECK-EL:      sll     $[[R5:[0-9]+]], $[[R3]], 3
+; CHECK-EB:      xori    $[[R4:[0-9]+]], $[[R3]], 2
+; CHECK-EB:      sll     $[[R5:[0-9]+]], $[[R4]], 3
+; ALL:           ori     $[[R6:[0-9]+]], $zero, 65535
+; ALL:           sllv    $[[R7:[0-9]+]], $[[R6]], $[[R5]]
+; ALL:           nor     $[[R8:[0-9]+]], $zero, $[[R7]]
+; ALL:           andi    $[[R9:[0-9]+]], $4, 65535
+; ALL:           sllv    $[[R10:[0-9]+]], $[[R9]], $[[R5]]
+; ALL:           andi    $[[R11:[0-9]+]], $5, 65535
+; ALL:           sllv    $[[R12:[0-9]+]], $[[R11]], $[[R5]]
+
+; ALL:       $[[BB0:[A-Z_0-9]+]]:
+; ALL:           ll      $[[R13:[0-9]+]], 0($[[R2]])
+; ALL:           and     $[[R14:[0-9]+]], $[[R13]], $[[R7]]
+; ALL:           bne     $[[R14]], $[[R10]], $[[BB1:[A-Z_0-9]+]]
+
+; ALL:           and     $[[R15:[0-9]+]], $[[R13]], $[[R8]]
+; ALL:           or      $[[R16:[0-9]+]], $[[R15]], $[[R12]]
+; ALL:           sc      $[[R16]], 0($[[R2]])
+; NOT-MICROMIPS: beqz    $[[R16]], $[[BB0]]
+; MICROMIPS:     beqzc   $[[R16]], $[[BB0]]
+
+; ALL:       $[[BB1]]:
+; ALL:           srlv    $[[R17:[0-9]+]], $[[R14]], $[[R5]]
+
+; ALL-NOT:    sll     $[[R18:[0-9]+]], $[[R17]], 16
+; ALL-NOT:    sra     $2, $[[R18]], 16
+
+; ALL-NOT:   seb     $2, $[[R17]]
+}
 
+; Check one signed i16 so that we cover the seh sign extend case
 define signext i16 @AtomicLoadAdd16(i16 signext %incr) nounwind {
 entry:
   %0 = atomicrmw add i16* @z, i16 %incr monotonic
   ret i16 %0
 
 ; ALL-LABEL: AtomicLoadAdd16:
 
 ; MIPS32-ANY:    lw      $[[R0:[0-9]+]], %got(z)