[PATCH] [mips] Handling of immediates bigger than 16 bits

[Daniel Sanders]

There's quite a few comments here but most are about fairly small issues.

> Resolves https://dmz-portal.mips.com/bugz/show_bug.cgi?id=2059

> 

> It appears that it also resolves:

>  https://dmz-portal.mips.com/bugz/show_bug.cgi?id=923

>  https://dmz-portal.mips.com/bugz/show_bug.cgi?id=689


As noted on http://reviews.llvm.org/D10537, the MIPS bugzilla isn't used by the LLVM community. Generally speaking, we shouldn't reference it.


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1650-1653
@@ +1649,6 @@
+  case Mips::XORi:
+    if ((Inst.getNumOperands() == 3) &&
+         Inst.getOperand(0).isReg() &&
+         Inst.getOperand(1).isReg() &&
+         Inst.getOperand(2).isImm()) {
+      int64_t ImmValue = Inst.getOperand(2).getImm();
----------------
I don't understand this check. All the above instructions have the form:
  insn reg, reg, imm
Can you show me a case where it's false?

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1655
@@ +1654,3 @@
+      int64_t ImmValue = Inst.getOperand(2).getImm();
+      return !(-32768 <= ImmValue && ImmValue <= 65535);
+    } else {
----------------
Not quite. For ANDi, ORi, and XORi, the range will be 0 to 65535, but for ADDi, ADDiu, SLTi, SLTiu it will be -32768 to 32767.

Also, use isInt<16>(ImmValue) and isUInt<16>(ImmValue) as appropriate.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1656-1658
@@ -1639,1 +1655,5 @@
+      return !(-32768 <= ImmValue && ImmValue <= 65535);
+    } else {
+        return false;
+    }
   default:
----------------
Nit: Don't use redundant braces or else-after-return.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2497
@@ +2496,3 @@
+  if (DstReg == SrcReg) {
+       ATReg = getATReg(Inst.getLoc());
+       FinalDstReg = DstReg;
----------------
What if ATReg is 0 because .set noat is in effect?

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2502
@@ +2501,3 @@
+
+  if (!loadImmediate(ImmValue, DstReg, Mips::NoRegister, true, Inst.getLoc(), Instructions)) {
+    switch (FinalOpcode) {
----------------
The 'true' seems suspicious. Am I right in assuming support for the 64-bit equivalents will be in a later patch?

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2547-2551
@@ +2546,7 @@
+    Instructions.push_back(tmpInst);
+  } else {
+    return true;
+  }
+
+  return false;
+}
----------------
Nit: Avoid return-after-else like so:
    ...
    Instructions.push_back(tmpInst);
    return false
  }
  return true;

================
Comment at: test/MC/Mips/instalias-imm-expanding.s:30
@@ +29,3 @@
+  add $4, $5, -0x80000000
+# CHECK: lui	$4, 32768               # encoding: [0x00,0x80,0x04,0x3c]
+# CHECK: add	$4, $4, $5              # encoding: [0x20,0x20,0x85,0x00]
----------------
Nit: Inconsistent indentation of the '# encoding'.

By the way, I'm thinking about removing most of these '# encoding' checks. If we've tested it properly in test/MC/Mips/*/valid.s then we don't really need to do it again. That's something for later though.

================
Comment at: test/MC/Mips/instalias-imm-expanding.s:293-297
@@ +292,6 @@
+# CHECK: xor	$4, $4, $5              # encoding: [0x26,0x20,0x85,0x00]
+
+
+
+# Force at least 8 (non-delay-slot) zero bytes, to make 'objdump' print ...
+    .space	8
----------------
Why? You don't check for it.

http://reviews.llvm.org/D10539

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/