[mips] Fix emitAtomicCmpSwapPartword to handle 64 bit pointers correctly

[zoran.jovanovic]

Summary

Updated revision of: http://reviews.llvm.org/D13649

Test atomic.ll modified because MipsInstrInfo::getEquivalentCompactForm does not handle mips64r6 instructions. Support should be added in separate patch.

[sdardis]

Test atomic.ll modified because MipsInstrInfo::getEquivalentCompactForm does not handle mips64r6 instructions. Support should be added in separate patch.

I'll be submitting a patch for that once I've addressed some test failures from lnt execution.

[mpf]

The changes in this patch look like they go too far. The issue was about pointer size not integer size. The only pseudo that needs to be i64 is alignedaddr when pointers are 64-bit and the only operation that needed to change was the AND that calculates alignedaddr. The rest should (and I believe must) stay as i32 as the values are used alongside LL/SC which operate on i32 types none of the data in this code exceeds 32-bit as it is all about sub-word operations.

[zoran.jovanovic]

As stated in description of the patch it is new revision of: http://reviews.llvm.org/D13649, which was created by Jelena Losic, and all following revisions were based on received comments.
This revision is based on latest set of comments received on http://reviews.llvm.org/D13649.
I do believe that it produces correct output.
Also, I do agree that particular issue can be fixed by only changing type of alignedaddr pseudo to i64 when pointers are 64 bit.
I will prepare new patch version that handles issue in that manner.

[zoran.jovanovic]

New patch version that only changes type of AlignedAddr and uses appropriate AND instruction for calculating value of AlignedAddr (although correct output will be produced with any AND instruction because they all work on register size data - functionally there is no AND32 and AND64).

[dsanders]

As Matthew mentioned, this new version has gone further than my comments intended. It wasn't my intention to promote every integer operation to i64, just the AND/OR/XOR operations (and my position on these operations needs to change following an off-list discussion about sign-extension on MIPS, see below). The majority should stick to the type of the result of Mips::LL, otherwise the Mips::LL will have incompatible registers attached to the instruction.

The discussion on sign-extension on MIPS has caused us to decide to keep i32 AND/OR/XOR as a legal operation on MIPS64 despite such an operation not existing on MIPS64 implementations. This is because of an architectural requirement for i32 operations to produce i32 results that have been sign-extended to GPR-width. As long as this requirement is met (and there is currently a bug in this area which Vasileios is fixing) then the GPR-width AND/OR/XOR operations will handle i32 values correctly in all cases.

lib/Target/Mips/MipsISelLowering.cpp

1492–1493 ↗

(On Diff #53326)

As mentioned in the comments on D13649, we're masking a pointer here so the predicate should use ArePtrs64bit instead of AreInts64bit. Some of the instructions that follow have the same issue (e.g. line 1494, 1498, 1502, etc.)

Similarly, MaskLSB2, PtrLSB2, Off, etc. are a pointer-sized integers and should use the RCp class.

1496–1501 ↗

(On Diff #53326)

It's not directly related to this patch but could you mention this optional xor in the above comment that summarizes the instructions the code will produce. It seems we forgot to do this in an earlier change

[dsanders]

To be clear, my comments are referring to Diff 53326 and not the latest version which was uploaded before I clicked 'submit'

[dsanders]

lib/Target/Mips/MipsISelLowering.cpp

1488–1489 ↗

(On Diff #53524)

On N64, this code can produce an AND64 with a GPR64 result, one GPR64 operand (Ptr), and a GPR32 operand (MaskLSB2). This contradicts the instruction definition for AND64 which expects the result and both operands to be GPR64.

Adding -verify-machineinstrs to your llc command will report the places where register classes are used incorrectly. You may also need to use SUBREG_TO_REG or EXTRACT_SUBREG to switch between register classes in some places.

[dsanders]

lib/Target/Mips/MipsISelLowering.cpp

1488–1489 ↗

(On Diff #53524)

Adding -verify-machineinstrs to your llc command will report the places where register classes are used incorrectly. You may also need to use SUBREG_TO_REG or EXTRACT_SUBREG to switch between register classes in some places.

Sorry, I've made a mistake here. EXTRACT_SUBREG is for an earlier stage of the code generator. At this stage you need to use something like BuildMI(...).addReg(Reg, 0, Mips::sub_32) instead.

[zoran.jovanovic]

New patch version.
Generated code is correct (sd/ld are used to store/restore pointer value).
It passes -verify-machineinstrs for o32, n32 and n64.

[dsanders]

LGTM with a couple small changes

lib/Target/Mips/MipsISelLowering.cpp

1491–1495 ↗

(On Diff #53554)

Both sides are nearly identical. Could you use addReg(Ptr, 0, ArePtrs64bit ? Mips::sub_32 : 0) to avoid the duplication?

test/CodeGen/Mips/atomicCmpSwapPW.ll

1–2 ↗

(On Diff #53554)

Please cover the O32 and N32 cases too.

4–5 ↗

(On Diff #53554)

Please add checks for the whole sequence and check we use the right instruction variant (-asm-show-inst will make this available to FileCheck).
Given that this patch is urgent and the test currently covers the most important detail, I'm ok with this particular bit being done as a follow-up.

llvm/trunk/lib/Target/Mips/MipsISelLowering.cpp

MipsTargetLowering::emitAtomicCmpSwapPartword(MachineInstr *MI,
                                               MachineBasicBlock *BB,
                                               unsigned Size) const {
   assert((Size == 1 || Size == 2) &&
       "Unsupported size for EmitAtomicCmpSwapPartial.");
 
   MachineFunction *MF = BB->getParent();
   MachineRegisterInfo &RegInfo = MF->getRegInfo();
   const TargetRegisterClass *RC = getRegClassFor(MVT::i32);
+  bool ArePtrs64bit = ABI.ArePtrs64bit();
+  const TargetRegisterClass *RCp =
+    getRegClassFor(ArePtrs64bit ? MVT::i64 : MVT::i32);
   const TargetInstrInfo *TII = Subtarget.getInstrInfo();
   DebugLoc DL = MI->getDebugLoc();
 
   unsigned Dest    = MI->getOperand(0).getReg();
   unsigned Ptr     = MI->getOperand(1).getReg();
   unsigned CmpVal  = MI->getOperand(2).getReg();
   unsigned NewVal  = MI->getOperand(3).getReg();
 
-  unsigned AlignedAddr = RegInfo.createVirtualRegister(RC);
+  unsigned AlignedAddr = RegInfo.createVirtualRegister(RCp);
   unsigned ShiftAmt = RegInfo.createVirtualRegister(RC);
   unsigned Mask = RegInfo.createVirtualRegister(RC);
   unsigned Mask2 = RegInfo.createVirtualRegister(RC);
   unsigned ShiftedCmpVal = RegInfo.createVirtualRegister(RC);
   unsigned OldVal = RegInfo.createVirtualRegister(RC);
   unsigned MaskedOldVal0 = RegInfo.createVirtualRegister(RC);
   unsigned ShiftedNewVal = RegInfo.createVirtualRegister(RC);
-  unsigned MaskLSB2 = RegInfo.createVirtualRegister(RC);
+  unsigned MaskLSB2 = RegInfo.createVirtualRegister(RCp);
   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);
MipsTargetLowering::emitAtomicCmpSwapPartword(MachineInstr *MI,
   loop2MBB->addSuccessor(sinkMBB);
   sinkMBB->addSuccessor(exitMBB);
 
   // FIXME: computation of newval2 can be moved to loop2MBB.
   //  thisMBB:
   //    addiu   masklsb2,$0,-4                # 0xfffffffc
   //    and     alignedaddr,ptr,masklsb2
   //    andi    ptrlsb2,ptr,3
+  //    xori    ptrlsb2,ptrlsb2,3              # Only for BE
   //    sll     shiftamt,ptrlsb2,3
   //    ori     maskupper,$0,255               # 0xff
   //    sll     mask,maskupper,shiftamt
   //    nor     mask2,$0,mask
   //    andi    maskedcmpval,cmpval,255
   //    sll     shiftedcmpval,maskedcmpval,shiftamt
   //    andi    maskednewval,newval,255
   //    sll     shiftednewval,maskednewval,shiftamt
   int64_t MaskImm = (Size == 1) ? 255 : 65535;
-  BuildMI(BB, DL, TII->get(Mips::ADDiu), MaskLSB2)
-    .addReg(Mips::ZERO).addImm(-4);
-  BuildMI(BB, DL, TII->get(Mips::AND), AlignedAddr)
+  BuildMI(BB, DL, TII->get(ArePtrs64bit ? Mips::DADDiu : Mips::ADDiu), MaskLSB2)
+    .addReg(ABI.GetNullPtr()).addImm(-4);
+  BuildMI(BB, DL, TII->get(ArePtrs64bit ? Mips::AND64 : Mips::AND), AlignedAddr)
     .addReg(Ptr).addReg(MaskLSB2);
-  BuildMI(BB, DL, TII->get(Mips::ANDi), PtrLSB2).addReg(Ptr).addImm(3);
+  BuildMI(BB, DL, TII->get(Mips::ANDi), PtrLSB2)
+      .addReg(Ptr, 0, ArePtrs64bit ? Mips::sub_32 : 0).addImm(3);
   if (Subtarget.isLittle()) {
     BuildMI(BB, DL, TII->get(Mips::SLL), ShiftAmt).addReg(PtrLSB2).addImm(3);
   } else {
     unsigned Off = RegInfo.createVirtualRegister(RC);
     BuildMI(BB, DL, TII->get(Mips::XORi), Off)
       .addReg(PtrLSB2).addImm((Size == 1) ? 3 : 2);
     BuildMI(BB, DL, TII->get(Mips::SLL), ShiftAmt).addReg(Off).addImm(3);
   }

llvm/trunk/test/CodeGen/Mips/atomicCmpSwapPW.ll

+; RUN: llc -O0 -march=mipsel -mcpu=mips32r2 -target-abi=o32 < %s -filetype=asm -o - \
+; RUN:   | FileCheck -check-prefix=PTR32 -check-prefix=ALL %s
+; RUN: llc -O0 -march=mips64el -mcpu=mips64r2 -target-abi=n32 < %s -filetype=asm -o - \
+; RUN:   | FileCheck  -check-prefix=PTR32 -check-prefix=ALL %s
+; RUN: llc -O0 -march=mips64el -mcpu=mips64r2 -target-abi=n64 < %s -filetype=asm -o - \
+; RUN:   | FileCheck -check-prefix=PTR64 -check-prefix=ALL %s
+
+; PTR32: lw $[[R0:[0-9]+]]
+; PTR64: ld $[[R0:[0-9]+]]
+
+; ALL: ll ${{[0-9]+}}, 0($[[R0]])
+
+define {i16, i1} @foo(i16* %addr, i16 signext %r, i16 zeroext %new) {
+  %res = cmpxchg i16* %addr, i16 %r, i16 %new seq_cst seq_cst
+  ret {i16, i1} %res
+}
+