[PATCH] D21615: [inlineasm][mips] Propagate operand constraints to the backend

[Daniel Sanders via llvm-commits]

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

This patch is going in the right direction but I think there's a couple extra things we ought to do to round this off:

- Stop MachineInstr::print() from printing register constraints when the flag do not hold a register constraint.
- Make MachineInstr::print() print the memory constraint when the flags have one.
- Add assertions to InlineAsm::getFlagWordForRegClass() and Inline::getFlagWordForMem() to make sure we cannot put the wrong kind of constraint in the relevant flag bits.

I also think the summary needs to mention that frameindex nodes introduce the out of range offsets that are causing the problem. Without the frameindex nodes we would have dealt with the out-of-range offsets during isel.


================
Comment at: lib/Target/Mips/MipsSERegisterInfo.cpp:66-93
@@ -65,20 +65,30 @@
 /// instruction immediate.
-static inline unsigned getLoadStoreOffsetSizeInBits(const unsigned Opcode) {
+static inline unsigned getLoadStoreOffsetSizeInBits(const unsigned Opcode,
+                                                    MachineOperand MO) {
+  unsigned ConstraintID;
   switch (Opcode) {
   case Mips::LD_B:
   case Mips::ST_B:
     return 10;
   case Mips::LD_H:
   case Mips::ST_H:
     return 10 + 1 /* scale factor */;
   case Mips::LD_W:
   case Mips::ST_W:
     return 10 + 2 /* scale factor */;
   case Mips::LD_D:
   case Mips::ST_D:
     return 10 + 3 /* scale factor */;
+  case Mips::INLINEASM:
+    ConstraintID  = InlineAsm::getMemoryConstraintID(MO.getImm());
+    switch (ConstraintID) {
+      case InlineAsm::Constraint_ZC:
+        return 9;
+      default:
+        return 16;
+    }
   default:
     return 16;
   }
 }
 
----------------
I don't mind if we do this in a separate patch but we need to cover the various versions of the LL/SC instructions in this switch statement too.

================
Comment at: lib/Target/Mips/MipsSERegisterInfo.cpp:68
@@ -67,1 +67,3 @@
+                                                    MachineOperand MO) {
+  unsigned ConstraintID;
   switch (Opcode) {
----------------
Scoping nit: Please move this into the 'case Mips::INLINE_ASM:', adding the necessary braces to allow this.

================
Comment at: lib/Target/Mips/MipsSERegisterInfo.cpp:86
@@ +85,3 @@
+      case InlineAsm::Constraint_ZC:
+        return 9;
+      default:
----------------
This value depends on the ISA. It should be 9-bit for MIPS32R6/MIPS64R6 and 16-bit for earlier ISA's.

================
Comment at: lib/Target/Mips/MipsSERegisterInfo.cpp:179
@@ -168,3 +178,3 @@
     // element size), otherwise it is a 16-bit signed immediate.
-    unsigned OffsetBitSize = getLoadStoreOffsetSizeInBits(MI.getOpcode());
+    unsigned OffsetBitSize = getLoadStoreOffsetSizeInBits(MI.getOpcode(), MI.getOperand(OpNo-1));
     unsigned OffsetAlign = getLoadStoreOffsetAlign(MI.getOpcode());
----------------
This is over 80 columns

================
Comment at: test/CodeGen/Mips/inlineasm-constraint_ZC_2.ll:1-2
@@ +1,3 @@
+; RUN: llc -march=mips -mcpu=mips32r6 < %s | FileCheck %s
+; RUN: llc -march=mips -mcpu=mips64r6 < %s | FileCheck %s
+
----------------
Please add a pre-R6 test to prove that 9-16 bit offsets are still accepted by the inline assembly for this case.

================
Comment at: test/CodeGen/Mips/inlineasm-constraint_ZC_2.ll:14-17
@@ +13,6 @@
+
+; CHECK: #APP
+; CHECK: ll ${{[0-9]+}}, 0(${{[0-9a-z]+}})
+; CHECK: sc ${{[0-9]+}}, 0(${{[0-9a-z]+}})
+; CHECK: #NO_APP
+
----------------
For the R6 case, could you check for the addiu too?


Repository:
  rL LLVM

http://reviews.llvm.org/D21615