[mips] Clang ll/sc illegal instruction on mips64r2 with -O0

[petarj]

Summary

This patch fixes an issue with storing/restoring pointers used by SC/LL instructions when N64 code is generated.
In case of N64 ABI sd/ld instructons should be used instead of sw/lw.
MipsTargetLowering::emitAtomicCmpSwapPartword is modified to use MVT::i64 (instead of MVT::i32) when creating virtual registers if code for N64 is generated.

[vkalintiris]

Please upload a diff with full context next time if you don't use arcanist (see http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface).

We should choose the correct instruction opcodes inside the function emitAtomicCmpSwapPartword() according to the register class that we want to use (see for example the code in function emitAtomicCmpSwap()).

lib/Target/Mips/MipsISelLowering.cpp

1415

We can use the ternary operator inside the getRegClassFor in order to save space. Also, make sure to clang-format the code.

test/CodeGen/Mips/at1.ll

1 ↗

(On Diff #37103)

You use %s twice here. We should pick only one method for passing the code to llc.

[Jelena.Losic]

This comment was removed by Jelena.Losic.

[Jelena.Losic]

I put code changes according to Vasileios suggestions.

[vkalintiris]

lib/Target/Mips/MipsISelLowering.cpp

1482–1512

We should specify the correct instructions, based on the register class, to every TII->get() in this function. For example, you could use something like that: unsigned NOR = Subtarget.isGP64bit() ? Mips::NOR64 : Mips::NOR; for every opcode we want to use.

test/CodeGen/Mips/at.ll

1–19 ↗

(On Diff #37471)

The test seems fine but could use a more meaningful filename, eg. test-atomic-cmp-swap.ll or something similar?

[Jelena.Losic]

In the method emitAtomicCmpPartword, when condition Subtaget.isGP64() is satisfied, there are included 64bit versions of atithmetic instructions.

[dsanders]

Most of the comments are nits, but the comment about the N32 case is important and potentially tricky to fix.

lib/Target/Mips/MipsISelLowering.cpp

1415–1420

Variables should start with a capital letter.

1416–1419

These decision should be based on the ABI's integer/pointer width rather than the CPU capabilities. Also, be careful about the difference between a pointer and an integer value on N32. N32's pointers need to use the 32-bit operations even though the registers holding them are 64-bit.

1483–1485

You can use ABI.GetZeroReg() to pick the right zero register

Just for completeness, pointers can use ABI.GetNullPtr()

1496–1497

Formatting

clang-format-diff.py can format the lines mentioned in the patch automatically. If you use it, make sure you give it zero lines of context so it doesn't format nearby code too.

1498–1499

Space before '?'. Similarly below

test/CodeGen/Mips/atomicCmpSwapPW.ll

3

Could you add a comment explaining why this -implicit-check-not is the only check?

As far as I can see you're checking that we don't spill/reload using sw/lw.

[Jelena.Losic]

Arithmetic instructions that are used are selected according to ABI's integer/pointer width.

[dsanders]

The latest patch only deals with part of the problem. I've commented on the first few pointer/integer inconsistencies but there are more.

lib/Target/Mips/MipsISelLowering.cpp

1415–1418

Nit: For consistency please use 'IsABIN64'. It will be confusing to use '64' and 'N64' interchangably. Or preferably, refer to the pointer size instead to make it easier to keep track of the difference between integer and pointer sizes.

1427–1443

Some of these should pick i64 for 64-bit integers (N32/N64), others should pick i64 for 64-bit pointers (N64 only).

1482–1483

This should pick between ADDiu and DADDiu depending on whether we have 32/64 bit pointers (at first it looks like an integer but it will be combined with a pointer in the next insn). You'll need to switch to GetNullPtr() too.

1490–1491

SLL/DSLL depending on pointer size

1496–1497

SLL/DSLL depending on pointer size

1498–1501

IsABI64 and GetZeroReg() do not agree with eachother.

1499–1501

SLL/DSLLV based on pointer size (I think)

1502–1503

SLLV/DSLLV. It's not immediately obvious whether this is integer or pointer

1502–1503

SLLV/DSLLV. It's not immediately obvious whether this is integer or pointer

1502–1503

IsABI64 and GetZeroReg() do not agree with eachother.

[Jelena.Losic]

I tried to distinguish width of integer/pointer used due to ABI.

[Jelena.Losic]

Daniel,
could you please give me some opinion about the latest changes in this patch, is this ok?

test/CodeGen/Mips/atomicCmpSwapPW.ll

3

I used -implicit-check-not just for lw, because after these changes in code llc doesn't emit any lw, but in one line emits sw instruction(which is correct, but -implicit-check-not sw will cause test fail).

[dsanders]

lib/Target/Mips/MipsISelLowering.cpp

1412–1413

Use ABI.ArePtrs64bit() and ABI.AreGprs64bit() instead.

1496–1498

One path considers ShiftAmt to be pointer-sized, the other integer-sized. This inconsistency needs to be fixed.

With that fixed, the shift left is the same on both paths. Given that PtrLSB2 is never used after this, it's possible to merge the common parts like this:

if (!Subtarget.isLittle()) {
  unsigned Off = RegInfo.createVirtualRegister(RCi);
  BuildMI(BB, DL, TII->get(ArePtrs64bit ? Mips::XORi64 : Mips::XORi), Off).addReg(PtrLSB2).addImm((Size == 1) ? 3 : 2);
  PtrLSB2 = Off;
}
BuildMI(BB, DL, TII->get(ArePtrs64bit ? Mips::DSLL : Mips::SLL), ShiftAmt).addReg(PtrLSB2).addImm(3);

1496–1499

We still have some type inconsistencies here and there are two ways of solving this. The first is consistent with the code generator as it is today, the second is more correct for the hardware (and the code generator should be changed to that approach).

MaskLSB2 should be pointer-sized since it is used as a mask for AlignedAddr which is a pointer. It follows that the assignment to MaskLSB2 must therefore use ABI.GetNullPtr() instead of ABI.GetZeroReg(). Similarly, PtrLSB2, Off, and ShiftAmt must be pointer-sized in this approach.

However, it's a little more correct to promote 'Ptr' to be GPR-sized since there is no 32-bit bitwise AND on MIPS64 (the AND opcode always operates on GPR-width values). If you take this approach then MaskLSB2, PtrLSB2, and Off must be GPR-sized and you must truncate PtrLSB2/Off before using it as an operand for the shift that results in ShiftAmt.

test/CodeGen/Mips/atomicCmpSwapPW.ll

3

I meant a comment in the test file.

I've just noticed that this test passes without your change because the code generator selects an ld and not an lw. I don't think this test covers the code change.

8–11

The personality, %exn.slot, and %ehselector.slot don't seem relevant to the test. Are they really needed?

[petarj]

AFAIU, the original problem has been fixed with http://reviews.llvm.org/D14397 , so we can abandon this change and close the issue.

[sdardis]

This minimal test case:

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
}

compiled for mips64r6 (big endian) with -target-abi=n64 -O0 shows the bug:
(irrelevant assembly removed)

$BB0_1:                               # =>This Inner Loop Header: Depth=1
      lw      $1, 36($sp)             # 4-byte Folded Reload
      ll      $2, 0($1)

Which is sufficient to check that the change is correct.

lib/Target/Mips/MipsISelLowering.cpp

1506–1511

Trailing whitespace on the lines with Mips::SLLV.

[sdardis]

Jelena, Can you mark this revision as abandoned, as D18995 fixes the issue? Thanks.

[petarj]

Abandoning the issue as the other change [1] fixed it.

[1] http://reviews.llvm.org/D18995 .

lib/Target/Mips/MipsISelLowering.cpp

 MachineBasicBlock *
 MipsTargetLowering::emitAtomicCmpSwapPartword(MachineInstr *MI,
                                               MachineBasicBlock *BB,
                                               unsigned Size) const {
   assert((Size == 1 || Size == 2) &&
       "Unsupported size for EmitAtomicCmpSwapPartial.");
 
   MachineFunction *MF = BB->getParent();
+  bool ArePtrs64bit = ABI.IsN64();
+  bool AreInts64bit = ABI.IsN64() || ABI.IsN32();
   MachineRegisterInfo &RegInfo = MF->getRegInfo();
-  const TargetRegisterClass *RC = getRegClassFor(MVT::i32);
+  llvm::MVT PointerSize = ArePtrs64bit ? MVT::i64 : MVT::i32;
+  llvm::MVT IntValSize = AreInts64bit ? MVT::i64 : MVT::i32;
+  const TargetRegisterClass *RC = getRegClassFor(PointerSize);
+  const TargetRegisterClass *RCi = getRegClassFor(IntValSize);
   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 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 ShiftAmt = RegInfo.createVirtualRegister(RCi);
+  unsigned Mask = RegInfo.createVirtualRegister(RCi);
+  unsigned Mask2 = RegInfo.createVirtualRegister(RCi);
+  unsigned ShiftedCmpVal = RegInfo.createVirtualRegister(RCi);
+  unsigned OldVal = RegInfo.createVirtualRegister(RCi);
+  unsigned MaskedOldVal0 = RegInfo.createVirtualRegister(RCi);
+  unsigned ShiftedNewVal = RegInfo.createVirtualRegister(RCi);
+  unsigned MaskLSB2 = RegInfo.createVirtualRegister(RCi);
   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 MaskUpper = RegInfo.createVirtualRegister(RCi);
+  unsigned MaskedCmpVal = RegInfo.createVirtualRegister(RCi);
+  unsigned MaskedNewVal = RegInfo.createVirtualRegister(RCi);
+  unsigned MaskedOldVal1 = RegInfo.createVirtualRegister(RCi);
   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);
   MachineFunction::iterator It = ++BB->getIterator();
MipsTargetLowering::emitAtomicCmpSwapPartword(MachineInstr *MI,
   //    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(AreInts64bit ? Mips::DADDiu : Mips::ADDiu), MaskLSB2)
+    .addReg(ABI.GetZeroReg()).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(ArePtrs64bit ? Mips::ANDi64 : Mips::ANDi), PtrLSB2)
+    .addReg(Ptr).addImm(3);
   if (Subtarget.isLittle()) {
-    BuildMI(BB, DL, TII->get(Mips::SLL), ShiftAmt).addReg(PtrLSB2).addImm(3);
+    BuildMI(BB, DL, TII->get(ArePtrs64bit ? Mips::DSLL : Mips::SLL), ShiftAmt)
+            .addReg(PtrLSB2).addImm(3);
   } else {
-    unsigned Off = RegInfo.createVirtualRegister(RC);
-    BuildMI(BB, DL, TII->get(Mips::XORi), Off)
+    unsigned Off = RegInfo.createVirtualRegister(RCi);
+    BuildMI(BB, DL, TII->get(ArePtrs64bit ? Mips::XORi64 : Mips::XORi), Off)
       .addReg(PtrLSB2).addImm((Size == 1) ? 3 : 2);
-    BuildMI(BB, DL, TII->get(Mips::SLL), ShiftAmt).addReg(Off).addImm(3);
+    BuildMI(BB, DL, TII->get(AreInts64bit ? Mips::DSLL : Mips::SLL), ShiftAmt)
+            .addReg(Off).addImm(3);
   }
-  BuildMI(BB, DL, TII->get(Mips::ORi), MaskUpper)
-    .addReg(Mips::ZERO).addImm(MaskImm);
-  BuildMI(BB, DL, TII->get(Mips::SLLV), Mask)
+  BuildMI(BB, DL, TII->get(AreInts64bit ? Mips::ORi64 : Mips::ORi), MaskUpper)
+    .addReg(ABI.GetZeroReg()).addImm(MaskImm);
+  BuildMI(BB, DL, TII->get(AreInts64bit ? Mips::DSLLV : Mips::SLLV), Mask)
     .addReg(MaskUpper).addReg(ShiftAmt);
-  BuildMI(BB, DL, TII->get(Mips::NOR), Mask2).addReg(Mips::ZERO).addReg(Mask);
-  BuildMI(BB, DL, TII->get(Mips::ANDi), MaskedCmpVal)
-    .addReg(CmpVal).addImm(MaskImm);
-  BuildMI(BB, DL, TII->get(Mips::SLLV), ShiftedCmpVal)
-    .addReg(MaskedCmpVal).addReg(ShiftAmt);
-  BuildMI(BB, DL, TII->get(Mips::ANDi), MaskedNewVal)
-    .addReg(NewVal).addImm(MaskImm);
-  BuildMI(BB, DL, TII->get(Mips::SLLV), ShiftedNewVal)
-    .addReg(MaskedNewVal).addReg(ShiftAmt);
+  BuildMI(BB, DL, TII->get(AreInts64bit ? Mips::NOR64 : Mips::NOR), Mask2)
+    .addReg(ABI.GetZeroReg()).addReg(Mask);
+  BuildMI(BB, DL, TII->get(AreInts64bit ? Mips::ANDi64 : Mips::ANDi),
+    MaskedCmpVal).addReg(CmpVal).addImm(MaskImm);
+  BuildMI(BB, DL, TII->get(AreInts64bit ? Mips::DSLLV : Mips::SLLV), 
+    ShiftedCmpVal).addReg(MaskedCmpVal).addReg(ShiftAmt);
+  BuildMI(BB, DL, TII->get(AreInts64bit ? Mips::ANDi64 : Mips::ANDi),
+    MaskedNewVal).addReg(NewVal).addImm(MaskImm);
+  BuildMI(BB, DL, TII->get(AreInts64bit ? Mips::DSLLV : Mips::SLLV), 
+    ShiftedNewVal).addReg(MaskedNewVal).addReg(ShiftAmt);
 
   //  loop1MBB:
   //    ll      oldval,0(alginedaddr)
   //    and     maskedoldval0,oldval,mask
   //    bne     maskedoldval0,shiftedcmpval,sinkMBB
   BB = loop1MBB;
   unsigned LL = isMicroMips ? Mips::LL_MM : Mips::LL;
   BuildMI(BB, DL, TII->get(LL), OldVal).addReg(AlignedAddr).addImm(0);
-  BuildMI(BB, DL, TII->get(Mips::AND), MaskedOldVal0)
-    .addReg(OldVal).addReg(Mask);
-  BuildMI(BB, DL, TII->get(Mips::BNE))
+  BuildMI(BB, DL, TII->get(AreInts64bit ? Mips::AND64 : Mips::AND),
+          MaskedOldVal0).addReg(OldVal).addReg(Mask);
+  BuildMI(BB, DL, TII->get(AreInts64bit ? Mips::BNE64 : Mips::BNE))
     .addReg(MaskedOldVal0).addReg(ShiftedCmpVal).addMBB(sinkMBB);
 
   //  loop2MBB:
   //    and     maskedoldval1,oldval,mask2
   //    or      storeval,maskedoldval1,shiftednewval
   //    sc      success,storeval,0(alignedaddr)
   //    beq     success,$0,loop1MBB
   BB = loop2MBB;
-  BuildMI(BB, DL, TII->get(Mips::AND), MaskedOldVal1)
-    .addReg(OldVal).addReg(Mask2);
-  BuildMI(BB, DL, TII->get(Mips::OR), StoreVal)
+  BuildMI(BB, DL, TII->get(AreInts64bit ? Mips::AND64 : Mips::AND),
+          MaskedOldVal1).addReg(OldVal).addReg(Mask2);
+  BuildMI(BB, DL, TII->get(AreInts64bit ? Mips::OR64 : 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);
+  BuildMI(BB, DL, TII->get(AreInts64bit ? Mips::BEQ64 : Mips::BEQ))
+      .addReg(Success).addReg(ABI.GetZeroReg()).addMBB(loop1MBB);
 
   //  sinkMBB:
   //    srl     srlres,maskedoldval0,shiftamt
   //    sign_extend dest,srlres
   BB = sinkMBB;
 
-  BuildMI(BB, DL, TII->get(Mips::SRLV), SrlRes)
+  BuildMI(BB, DL, TII->get(AreInts64bit ? Mips::DSRLV : Mips::SRLV), SrlRes)
       .addReg(MaskedOldVal0).addReg(ShiftAmt);
   BB = emitSignExtendToI32InReg(MI, BB, Size, Dest, SrlRes);
 
   MI->eraseFromParent();   // The instruction is gone now.
 
   return exitMBB;
 }
 

test/CodeGen/Mips/atomicCmpSwapPW.ll

+; RUN: llc  -O0 -march=mips64el -mcpu=mips64r2 < %s -filetype=asm -o - \
+; RUN:   | FileCheck %s -implicit-check-not=lw
+
+@_ZZ14InitializeOncevE5array = global [1 x i32*] zeroinitializer, align 8
+@_ZGVZ14InitializeOncevE5array = global i64 0, align 8
+
+define void @_Z14InitializeOncev()
+#0 personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
+entry:
+  %exn.slot = alloca i8*
+  %ehselector.slot = alloca i32
+  %0 = load atomic i8,
+     i8* bitcast (i64* @_ZGVZ14InitializeOncevE5array to i8*) acquire, align 8
+  ret void
+}
+
+
+declare i32 @__gxx_personality_v0(...)
 No newline at end of file