[PATCH] D14390: [mips] Expansion of LI.S and LI.D

[Simon Dardis via llvm-commits]

sdardis added inline comments.


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2769
+  case Mips::RA:   return Mips::ZERO;
+  default:         return 0;
+  }
----------------
Two things:

This function also needs to handle floating point registers.

Rather than returning 0, instead call llvm_unreachable("Unknown register in assembly macro expansion!");


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2929
+
+    if (LoImmOp64 == 0) {
+      unsigned ATReg = getATReg(IDLoc);
----------------
This block has the wrong condition. This should check that the lower 32 bits are zero and the high part can be loaded with a single instruction.


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2931-2936
+      if (!ATReg)
+        return true;
+      if (!Is64FPU) {
+        if (loadImmediate(HiImmOp64, ATReg, Mips::NoRegister, true, true, IDLoc,
+                          Out, STI))
+          return true;
----------------
Add a FIXME comment here noting that in the case where the constant is zero, we can load the register directly from the zero register.


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2933
+        return true;
+      if (!Is64FPU) {
+        if (loadImmediate(HiImmOp64, ATReg, Mips::NoRegister, true, true, IDLoc,
----------------
This condition is not required.


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2937
+          return true;
+        if(hasMips32r2()) {
+          TOut.emitRRR(Mips::MTHC1_D32, FirstReg, FirstReg, ATReg, IDLoc, STI);
----------------
This also needs a check for hasMips64() before checking hasMips32r2(). In the mips64 case, we use dmtc1.


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2938-2939
+        if(hasMips32r2()) {
+          TOut.emitRRR(Mips::MTHC1_D32, FirstReg, FirstReg, ATReg, IDLoc, STI);
+          TOut.emitRR(Mips::MTC1, FirstReg, Mips::ZERO, IDLoc, STI);
+        } else {
----------------
If you've reverse engineered this from the output of gas, you've hit upon a latent gas bug.

On a MIPS32 system (or a MIPS64 system executing MIPS32 code) , writing to a 64 bit FPU register must be done in a specific order; use mtc1 to write the lower 32bits, then use mthc1 to write the upper 32 bits. mtcX instructions leave the upper 32bits *UNPREDICTABLE*.

Reverse the order of these two lines.


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2941-2942
+        } else {
+         TOut.emitRR(Mips::MTC1, nextReg(FirstReg), ATReg, IDLoc, STI);
+         TOut.emitRR(Mips::MTC1, FirstReg, Mips::ZERO, IDLoc, STI);
+        }
----------------
See my comment about nextReg.

Also, indentation is incorrect.


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2979-2980
+
+    TOut.emitRRX(Mips::LDC1, FirstReg, Mips::GP,
+                 MCOperand::createExpr(FixupAddr), IDLoc, STI);
+  }
----------------
Rather than Mips::LDC1, this should be (IsFPU64 ? Mips::LDC164 : Mips::LDC1) to get the correct instruction.

Add a FIXME comment here noting that this expansion is incorrect for mips1, it should expand to two word loads.


================
Comment at: test/MC/Mips/macro-li.d.s:1
+# RUN: llvm-mc  %s -triple=mipsel-unknown-linux -mcpu=mips32r2 -show-encoding | FileCheck %s --check-prefix=CHECK-MIPS32
+# RUN: llvm-mc  %s -triple=mipsel-unknown-linux -mcpu=mips64r2 -show-encoding | FileCheck %s --check-prefix=CHECK-MIPS64
----------------
Can you also a RUN line for mips32 and add appropriate tests?


https://reviews.llvm.org/D14390