[mips] [IAS] Fix expansion of negative 32-bit immediates for LI/DLI.

[tomatabacu]

Summary

To maintain compatibility with GAS, we need to stop treating negative 32-bit immediates as 64-bit values when expanding LI/DLI.
This currently happens because of sign extension.

To do this we need to choose the 32-bit value expansion for values which use their upper 33 bits only for sign extension (i.e. no 0's, only 1's).

[dsanders]

I think we're attacking this in the wrong place at the moment. It's difficult to tell without going deep into the guts of the matcher but it's important to the operation of the parser that we don't specialize our operands too early since there's no facility for backtracking.

This was a particular problem for floating point registers a while back. The parser would identify register names like '$f0' as being an F0_64 and having committed to that interpretation would then fail to match FGR32 versions of the instructions. In our current implementation, our floating point operand for $f0 is all three of F0, D0, and F0_64 (it's stored as a register index that permits FGR32, FGR64, and AFGR64 contexts) until the operand is rendered into an instruction which specializes it to a single register. The same needs to be true of integers, we need to have a generic 64-bit integer which is specialized into other integer types when it's rendered into an instruction.

If the instruction were expanded by the tablegen-erated matcher, we could attack this in tablegen using an AsmOperandClass. The PredicateMethod would determine whether the value was a valid unsigned/signed 32-bit integer (which is the case when it fits in a _33_-bit signed integer). Then if the instruction matched, the RenderMethod would adjust the value so that the excess bits are a sign-extension of the 32-bit value. So for example, 0x0000000080000000 would return true for the PredicateMethod and be converted to 0xffffffff80000000 by the RenderMethod.

I'd therefore suggest that expandLoadImm() have some way of knowing whether the context for the integer is 32-bit or 64-bit. When called in a 32-bit context, it would convert the value to a signed 32-bit integer before emitting the code to produce it.

[tomatabacu]

Took a new approach to solving the problem.
Updated title and summary to reflect code changes.
Updated tests.

[tomatabacu]

Ping.

[dsanders]

Thanks for reworking this. A couple comments:

lib/Target/Mips/AsmParser/MipsAsmParser.cpp

1734–1735 ↗

(On Diff #23838)

This condition is a complicated way of saying 'does it fit in a signed/unsigned 32-bit integer?' which can be more simply written as 'isInt<32>(ImmValue) || isUInt<32>(ImmValue)'. A few of the other paths do the same kind of thing, for example the first if-statement could be written 'isUInt<16>(ImmValue).

Also: it isn't correct for the DLI case when ImmValue is 0x80000000 because 'lui' produces a sign-extended (32->64-bit) result giving 0xffffffff80000000 in the register. It's fine for 32-bit though.
You need the condition to be isInt<32>(ImmValue) for the DLI case and 'isInt<32>(ImmValue) || isUInt<32>(ImmValue)' for the LI case.

1755 ↗

(On Diff #23838)

I know this isn't from your patch but I just noticed it. Shouldn't these be '16-bits'?

1780 ↗

(On Diff #23838)

Same here

[tomatabacu]

Replied to inline comments.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp

1755 ↗

(On Diff #23838)

Yes. Fixed in r235891.

1780 ↗

(On Diff #23838)

Yes. Fixed in r235891.

[tomatabacu]

Switched to using the templates from MathExtras.h.
Switched to using ORi instead of LUi for DLI, to avoid sign-extension.
Rebased on top of D8974, D9289 and D9290 to make things easier to do.

Will switch the other paths to use the templates from MathExtras.h in a follow-up patch.

[tomatabacu]

Rebased on top of new D9290.

[dsanders]

With a couple corrections it will LGTM

lib/Target/Mips/AsmParser/MipsAsmParser.cpp

1762 ↗

(On Diff #25146)

This isn't quite right. You need to check for `!isInt<32>(ImmValue)`. That way it will use lui+ori except when it really needs to use ori+dslli+ori such as for 0x80000000.

You'll also need a test to cover this once the check is corrected.

test/MC/Mips/mips64-expansions.s

13–14 ↗

(On Diff #25146)

This was correct before. 0x10002 should use:
lui ..., 0x1
ori ..., ..., 0x2

21–22 ↗

(On Diff #25146)

This was correct before. 0x10000 should use
lui ..., 0x1

[tomatabacu]

Made the requested changes.

llvm/trunk/lib/Target/Mips/AsmParser/MipsAsmParser.cpp

if (0 <= ImmValue && ImmValue <= 65535) {
     // li d,j => addiu d,$zero,j
     if (!UseSrcReg)
       SrcReg = Mips::ZERO;
     tmpInst.setOpcode(Mips::ADDiu);
     tmpInst.addOperand(MCOperand::createReg(DstReg));
     tmpInst.addOperand(MCOperand::createReg(SrcReg));
     tmpInst.addOperand(MCOperand::createImm(ImmValue));
     Instructions.push_back(tmpInst);
-  } else if ((ImmValue & 0xffffffff) == ImmValue) {
+  } else if (isInt<32>(ImmValue) || isUInt<32>(ImmValue)) {
     if (!AssemblerOptions.back()->isMacro())
       Warning(IDLoc, "macro instruction expanded into multiple instructions");
 
     // For all other values which are representable as a 32-bit integer:
     // li d,j => lui d,hi16(j)
     //           ori d,d,lo16(j)
     uint16_t Bits31To16 = (ImmValue >> 16) & 0xffff;
     uint16_t Bits15To0 = ImmValue & 0xffff;
 
+    if (!Is32BitImm && !isInt<32>(ImmValue)) {
+      // For DLI, expand to an ORi instead of a LUi to avoid sign-extending the
+      // upper 32 bits.
+      tmpInst.setOpcode(Mips::ORi);
+      tmpInst.addOperand(MCOperand::createReg(DstReg));
+      tmpInst.addOperand(MCOperand::createReg(Mips::ZERO));
+      tmpInst.addOperand(MCOperand::createImm(Bits31To16));
+      tmpInst.setLoc(IDLoc);
+      Instructions.push_back(tmpInst);
+      // Move the value to the upper 16 bits by doing a 16-bit left shift.
+      createLShiftOri<16>(0, DstReg, IDLoc, Instructions);
+    } else {
       tmpInst.setOpcode(Mips::LUi);
       tmpInst.addOperand(MCOperand::createReg(DstReg));
       tmpInst.addOperand(MCOperand::createImm(Bits31To16));
       Instructions.push_back(tmpInst);
+    }
     createLShiftOri<0>(Bits15To0, DstReg, IDLoc, Instructions);
 
     if (UseSrcReg)
       createAddu(DstReg, DstReg, SrcReg, Instructions);
 
   } else if ((ImmValue & (0xffffLL << 48)) == 0) {
     if (Is32BitImm) {
       Error(IDLoc, "instruction requires a 32-bit immediate");

llvm/trunk/test/MC/Mips/mips-expansions.s

 # RUN: llvm-mc %s -triple=mipsel-unknown-linux -show-encoding -mcpu=mips32r2 | FileCheck %s
 # Check that the assembler can handle the documented syntax
 # for macro instructions
 #------------------------------------------------------------------------------
 # Load immediate instructions
 #------------------------------------------------------------------------------
 # CHECK:     ori     $5, $zero, 123   # encoding: [0x7b,0x00,0x05,0x34]
 # CHECK:     addiu   $6, $zero, -2345 # encoding: [0xd7,0xf6,0x06,0x24]
 # CHECK:     lui     $7, 1            # encoding: [0x01,0x00,0x07,0x3c]
 # CHECK:     ori     $7, $7, 2        # encoding: [0x02,0x00,0xe7,0x34]
 # CHECK:     addiu   $8, $zero, -8    # encoding: [0xf8,0xff,0x08,0x24]
 # CHECK:     lui     $9, 1            # encoding: [0x01,0x00,0x09,0x3c]
 # CHECK-NOT: ori $9, $9, 0            # encoding: [0x00,0x00,0x29,0x35]
+# CHECK:     lui     $10, 65519       # encoding: [0xef,0xff,0x0a,0x3c]
+# CHECK:     ori     $10, $10, 61423  # encoding: [0xef,0xef,0x4a,0x35]
 
 # CHECK: ori     $4, $zero, 20       # encoding: [0x14,0x00,0x04,0x34]
 # CHECK: lui     $7, 1               # encoding: [0x01,0x00,0x07,0x3c]
 # CHECK: ori     $7, $7, 2           # encoding: [0x02,0x00,0xe7,0x34]
 # CHECK: ori     $4, $5, 20          # encoding: [0x14,0x00,0xa4,0x34]
 # CHECK: lui     $7, 1               # encoding: [0x01,0x00,0x07,0x3c]
 # CHECK: ori     $7, $7, 2           # encoding: [0x02,0x00,0xe7,0x34]
 # CHECK: addu    $7, $7, $8          # encoding: [0x21,0x38,0xe8,0x00]

 # CHECK: lui     $1, %hi(symbol)
 # CHECK: sdc1    $f0, %lo(symbol)($1)
 
     li $5,123
     li $6,-2345
     li $7,65538
     li $8, ~7
     li $9, 0x10000
+    li $10, ~(0x101010)
 
     la $a0, 20
     la $7,65538
     la $a0, 20($a1)
     la $7,65538($8)
     la $t0, symbol
 
     .set noat

llvm/trunk/test/MC/Mips/mips64-expansions.s

# CHECK: ori    $8, $8, 65535       # encoding: [0xff,0xff,0x08,0x35]
 
   dli $8, -0x1000100010001
 # CHECK: lui    $8, 65534           # encoding: [0xfe,0xff,0x08,0x3c]
 # CHECK: ori    $8, $8, 65534       # encoding: [0xfe,0xff,0x08,0x35]
 # CHECK: dsll   $8, $8, 16          # encoding: [0x38,0x44,0x08,0x00]
 # CHECK: ori    $8, $8, 65534       # encoding: [0xfe,0xff,0x08,0x35]
 # CHECK: dsll   $8, $8, 16          # encoding: [0x38,0x44,0x08,0x00]
 # CHECK: ori    $8, $8, 65535       # encoding: [0xff,0xff,0x08,0x35]
+
+# Check that signed negative 32-bit immediates are loaded correctly:
+  li $10, ~(0x101010)
+# CHECK: lui $10, 65519        # encoding: [0xef,0xff,0x0a,0x3c]
+# CHECK: ori $10, $10, 61423   # encoding: [0xef,0xef,0x4a,0x35]
+# CHECK-NOT: dsll
+
+  dli $10, ~(0x202020)
+# CHECK: lui $10, 65503        # encoding: [0xdf,0xff,0x0a,0x3c]
+# CHECK: ori $10, $10, 57311   # encoding: [0xdf,0xdf,0x4a,0x35]
+# CHECK-NOT: dsll
+
+  dli $9, 0x80000000
+# CHECK: ori  $9, $zero, 32768 # encoding: [0x00,0x80,0x09,0x34]
+# CHECK: dsll $9, $9, 16       # encoding: [0x38,0x4c,0x09,0x00]