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

[Daniel Sanders via llvm-commits]

dsanders requested changes to this revision.
This revision now requires changes to proceed.

================
Comment at: lib/Target/Mips/MipsISelLowering.cpp:1412-1413
@@ -1411,2 +1411,4 @@
   MachineFunction *MF = BB->getParent();
+  bool ArePtrs64bit = ABI.IsN64();
+  bool AreInts64bit = ABI.IsN64() || ABI.IsN32();
   MachineRegisterInfo &RegInfo = MF->getRegInfo();
----------------
Use ABI.ArePtrs64bit() and ABI.AreGprs64bit() instead.

================
Comment at: lib/Target/Mips/MipsISelLowering.cpp:1496-1498
@@ -1488,3 +1495,5 @@
       .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(AreInts64bit ? Mips::ORi64 : Mips::ORi), MaskUpper)
----------------
dsanders wrote:
> 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.
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);

================
Comment at: lib/Target/Mips/MipsISelLowering.cpp:1496-1499
@@ -1488,6 +1495,6 @@
       .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);
----------------
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.

================
Comment at: test/CodeGen/Mips/atomicCmpSwapPW.ll:3
@@ +2,3 @@
+; RUN:   | FileCheck %s -implicit-check-not=lw
+
+ at _ZZ14InitializeOncevE5array = global [1 x i32*] zeroinitializer, align 8
----------------
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.

================
Comment at: test/CodeGen/Mips/atomicCmpSwapPW.ll:8-11
@@ +7,6 @@
+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,
----------------
The personality, %exn.slot, and %ehselector.slot don't seem relevant to the test. Are they really needed?


http://reviews.llvm.org/D13649