[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

1744–1746

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.

1765

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

1791

Same here

[tomatabacu]

Replied to inline comments.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp

1765

Yes. Fixed in r235891.

1791

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

1752

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

2–3

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

2–3

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

[tomatabacu]

Made the requested changes.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp

public:
   /// This is typically only used for named registers such as $msaaccess.
   static std::unique_ptr<MipsOperand>
   createMSACtrlReg(unsigned Index, const MCRegisterInfo *RegInfo, SMLoc S,
                    SMLoc E, MipsAsmParser &Parser) {
     return CreateReg(Index, RegKind_MSACtrl, RegInfo, S, E, Parser);
   }
 
   static std::unique_ptr<MipsOperand>
-  CreateImm(const MCExpr *Val, SMLoc S, SMLoc E, MipsAsmParser &Parser) {
+  CreateImm(const MCExpr *Val, SMLoc S, SMLoc E, MipsAsmParser &Parser, bool IsNegated = false) {
     auto Op = make_unique<MipsOperand>(k_Immediate, Parser);
+    const MCConstantExpr *ValCstExpr = static_cast<const MCConstantExpr *>(Val);
+    int64_t ValInt = ValCstExpr->getValue();
+
+    // We don't clear the upper 32 bits for 16-bit negated values.
+    if (IsNegated && !(ValInt < 0 && ValInt >= -32768)) {
+      // Clear the upper 32 bits.
+      uint32_t NewValInt = ValInt & 0xffffffff;
+
+      // Replace the previous Value with our NewVal.
+      const MCConstantExpr *NewValConstExpr = MCConstantExpr::Create(
+          NewValInt, Parser.getContext());
+      Val = NewValConstExpr;
+    }
     Op->Imm.Val = Val;
     Op->StartLoc = S;
     Op->EndLoc = E;
     return Op;
   }
 
   static std::unique_ptr<MipsOperand>
   CreateMem(std::unique_ptr<MipsOperand> Base, const MCExpr *Off, SMLoc S,
bool MipsAsmParser::expandLoadImm(MCInst &Inst, SMLoc IDLoc,
   } else if (ImmValue < 0 && ImmValue >= -32768) {
     // For -32768 <= j < 0.
     // li d,j => addiu d,$zero,j
     tmpInst.setOpcode(Mips::ADDiu);
     tmpInst.addOperand(MCOperand::CreateReg(RegOp.getReg()));
     tmpInst.addOperand(MCOperand::CreateReg(Mips::ZERO));
     tmpInst.addOperand(MCOperand::CreateImm(ImmValue));
     Instructions.push_back(tmpInst);
   } else if ((ImmValue & 0xffffffff) == ImmValue) {
     // For any value of j that is representable as a 32-bit integer, create
     // a sequence of:
     // li d,j => lui d,hi16(j)
     //           ori d,d,lo16(j)
     tmpInst.setOpcode(Mips::LUi);
     tmpInst.addOperand(MCOperand::CreateReg(RegOp.getReg()));
     tmpInst.addOperand(MCOperand::CreateImm((ImmValue & 0xffff0000) >> 16));
     Instructions.push_back(tmpInst);
     createShiftOr<0, false>(ImmValue, RegOp.getReg(), IDLoc, Instructions);
   } else if ((ImmValue & (0xffffLL << 48)) == 0) {
     if (!isGP64bit()) {
       Error(IDLoc, "instruction requires a 64-bit architecture");
       return true;
     }
 
     //            <-------  lo32 ------>
     // <-------  hi32 ------>
     // <- hi16 ->             <- lo16 ->
     //  _________________________________
     // |          |          |          |
     // | 16-bytes | 16-bytes | 16-bytes |
     // |__________|__________|__________|
     //
     // For any value of j that is representable as a 48-bit integer, create
     // a sequence of:
     // li d,j => lui d,hi16(j)
     //           ori d,d,hi16(lo32(j))
     //           dsll d,d,16
     //           ori d,d,lo16(lo32(j))
if (!isGP64bit()) {
       Error(IDLoc, "instruction requires a 64-bit architecture");
       return true;
     }
 
     // <-------  hi32 ------> <-------  lo32 ------>
     // <- hi16 ->                        <- lo16 ->
     //  ___________________________________________
     // |          |          |          |          |
     // | 16-bytes | 16-bytes | 16-bytes | 16-bytes |
     // |__________|__________|__________|__________|
     //
     // For any value of j that isn't representable as a 48-bit integer.
     // li d,j => lui d,hi16(j)
     //           ori d,d,lo16(hi32(j))
     //           dsll d,d,16
     //           ori d,d,hi16(lo32(j))
     //           dsll d,d,16
MipsAsmParser::parseImm(OperandVector &Operands) {
   case AsmToken::Minus:
   case AsmToken::Plus:
   case AsmToken::Integer:
   case AsmToken::Tilde:
   case AsmToken::String:
     break;
   }
 
+  // If the immediate was negated, we clear the upper 32 bits of the negated
+  // value (unless it's a 16-bit value).
+  bool IsNegated = false;
+  if (getLexer().is(AsmToken::Tilde))
+    IsNegated = true;
+
   const MCExpr *IdVal;
   SMLoc S = Parser.getTok().getLoc();
   if (getParser().parseExpression(IdVal))
     return MatchOperand_ParseFail;
 
   SMLoc E = SMLoc::getFromPointer(Parser.getTok().getLoc().getPointer() - 1);
-  Operands.push_back(MipsOperand::CreateImm(IdVal, S, E, *this));
+  Operands.push_back(MipsOperand::CreateImm(IdVal, S, E, *this, IsNegated));
   return MatchOperand_Success;
 }
 
 MipsAsmParser::OperandMatchResultTy
 MipsAsmParser::parseJumpTarget(OperandVector &Operands) {
   MCAsmParser &Parser = getParser();
   DEBUG(dbgs() << "parseJumpTarget\n");
 

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     $10, 65519          # encoding: [0xef,0xff,0x0a,0x3c]
+# CHECK: ori     $10, $10, 61423     # encoding: [0xef,0xef,0x4a,0x35]
 
 # CHECK: addiu   $4, $zero, 20       # encoding: [0x14,0x00,0x04,0x24]
 # CHECK: lui     $7, 1               # encoding: [0x01,0x00,0x07,0x3c]
 # CHECK: ori     $7, $7, 2           # encoding: [0x02,0x00,0xe7,0x34]
 # CHECK: addiu   $4, $5, 20          # encoding: [0x14,0x00,0xa4,0x24]
 # 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: ldc1    $f0, %lo(symbol)($1)
 # CHECK: lui     $1, %hi(symbol)
 # CHECK: sdc1    $f0, %lo(symbol)($1)
 
     li $5,123
     li $6,-2345
     li $7,65538
     li $8, ~7
+    li $10, ~(0x101010)
 
     la $a0, 20
     la $7,65538
     la $a0, 20($a1)
     la $7,65538($8)
     la $t0, symbol
     .set mips64
     la $t0, symbol

test/MC/Mips/mips64-expansions.s

 # RUN: llvm-mc %s -triple=mips64el-unknown-linux -show-encoding -mcpu=mips64r2 | FileCheck %s
 #
 # The GNU assembler implements 'dli' and 'dla' variants on 'li' and 'la'
 # supporting double-word lengths.  Test that not only are they present, bu
 # that they also seem to handle 64-bit values.
 #
 # XXXRW: Does using powers of ten make me a bad person?
 #
 # CHECK: ori  $12, $zero, 1           # encoding: [0x01,0x00,0x0c,0x34]
 # CHECK: ori  $12, $zero, 10          # encoding: [0x0a,0x00,0x0c,0x34]
 # CHECK: ori  $12, $zero, 100         # encoding: [0x64,0x00,0x0c,0x34]
# CHECK: ori  $12, $12, 0             # encoding: [0x00,0x00,0x8c,0x35]
   dli $t0, -1000000000000
   dli $t0, -10000000000000
   dli $t0, -100000000000000
   dli $t0, -1000000000000000
   dli $t0, -10000000000000000
   dli $t0, -100000000000000000
   dli $t0, -1000000000000000000
   dli $t0, -10000000000000000000
+
+  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