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

[Daniel Sanders via llvm-commits]

dsanders requested changes to this revision.
dsanders added a reviewer: dsanders.
dsanders added a comment.
This revision now requires changes to proceed.

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


================
Comment at: lib/Target/Mips/MipsISelLowering.cpp:1407-1411
@@ -1406,4 +1406,7 @@
   MachineRegisterInfo &RegInfo = MF->getRegInfo();
-  const TargetRegisterClass *RC = getRegClassFor(MVT::i32);
+  bool isABI64 = Subtarget.isABI_N64();
+  const TargetRegisterClass *RC = 
+    getRegClassFor(isABI64 ? MVT::i64 : MVT::i32);
   const TargetInstrInfo *TII = Subtarget.getInstrInfo();
+  bool is64 =  Subtarget.isGP64bit();
   DebugLoc DL = MI->getDebugLoc();
----------------
Variables should start with a capital letter.

================
Comment at: lib/Target/Mips/MipsISelLowering.cpp:1408-1411
@@ -1408,2 +1407,6 @@
+  bool isABI64 = Subtarget.isABI_N64();
+  const TargetRegisterClass *RC = 
+    getRegClassFor(isABI64 ? MVT::i64 : MVT::i32);
   const TargetInstrInfo *TII = Subtarget.getInstrInfo();
+  bool is64 =  Subtarget.isGP64bit();
   DebugLoc DL = MI->getDebugLoc();
----------------
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.

================
Comment at: lib/Target/Mips/MipsISelLowering.cpp:1476
@@ -1472,3 +1475,3 @@
   BuildMI(BB, DL, TII->get(Mips::ADDiu), MaskLSB2)
-    .addReg(Mips::ZERO).addImm(-4);
-  BuildMI(BB, DL, TII->get(Mips::AND), AlignedAddr)
+    .addReg(is64 ? Mips::ZERO_64 : Mips::ZERO).addImm(-4);
+  BuildMI(BB, DL, TII->get(is64 ? Mips::AND64 : Mips::AND), AlignedAddr)
----------------
You can use ABI.GetZeroReg() to pick the right zero register

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

================
Comment at: lib/Target/Mips/MipsISelLowering.cpp:1486
@@ -1482,3 +1485,3 @@
       .addReg(PtrLSB2).addImm((Size == 1) ? 3 : 2);
-    BuildMI(BB, DL, TII->get(Mips::SLL), ShiftAmt).addReg(Off).addImm(3);
+    BuildMI(BB, DL, TII->get( Mips::SLL), ShiftAmt).addReg(Off).addImm(3);
   }
----------------
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.

================
Comment at: lib/Target/Mips/MipsISelLowering.cpp:1488
@@ -1484,3 +1487,3 @@
   }
-  BuildMI(BB, DL, TII->get(Mips::ORi), MaskUpper)
+  BuildMI(BB, DL, TII->get(is64? Mips::ORi64 : Mips::ORi), MaskUpper)
     .addReg(Mips::ZERO).addImm(MaskImm);
----------------
Space before '?'. Similarly below

================
Comment at: test/CodeGen/Mips/atomicCmpSwapPW.ll:2
@@ +1,3 @@
+; RUN: llc  -O0 -march=mips64el -mcpu=mips64r2 < %s -filetype=asm -o - \
+; RUN:   | FileCheck %s -implicit-check-not=lw
+
----------------
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.


http://reviews.llvm.org/D13649