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

[Simon Dardis via llvm-commits]

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

In several places there is the expression 'FirstReg + 1', This is unsafe as tablegen does not order the registers in the namespace as you would expect. Instead write a helper function to compute the next register.

Currently lib/Target/Mips/MipsGenRegisterInfo.inc in the build directory, the next register after a3 is ac0, not t0.

The second issue is that the mthc0 is only available for MIPS32r2 or later. For earlier revisions, the expansion is to use two mtcs, accessing the next numerical register and treating f0 as the "next register" in the f31 case.



================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:197-201
+  
+  bool expandLoadImmReal(MCInst &Inst, bool IsSingle, bool IsGPR, bool Is64FPU,
+                         SMLoc IDLoc, MCStreamer &Out,
+                         const MCSubtargetInfo *STI);
 
----------------
There appears to be spurious white space on the line before this prototype, please remove it when you're committing.


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:921
   }
+  
+  void addStrictlyAFGR64AsmRegOperands(MCInst &Inst, unsigned N) const {
----------------
Here too.


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:956
+  }
+  
   void addFGRH32AsmRegOperands(MCInst &Inst, unsigned N) const {
----------------
Whitespace here too.


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2196
                                                           : MER_Success;
+    
+  case Mips::LoadImmSingleGPR:
----------------
Here as well.


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2750-2756
+    // Convert double ImmOp64 into single ImmOp32
+    void *void_pt = static_cast<void *>(&ImmOp64);
+    double *double_pt = static_cast<double *>(void_pt);
+    float tmp_float = static_cast<float>(*double_pt);
+    void_pt = &tmp_float;
+    uint32_t *uint32_pt = static_cast<uint32_t *>(void_pt);
+    ImmOp32 = *uint32_pt;
----------------
Please adjust the comment to say that this is a conversion of a double in an uint64_t to a float in a uint32_t, retaining the bit pattern of a float.


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2805
+
+     TOut.emitRRX(Mips::LWC1, FirstReg, Mips::GP,
+                   MCOperand::createExpr(FixupAddr), IDLoc, STI);
----------------
Indentation, this needs another space before the TOut.emitRRX(Mips::LWC1, ...


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2830-2836
+    MCSection *CS = getStreamer().getCurrentSectionOnly();
+    MCSection *ReadOnlySection = getContext().getELFSection(
+        ".rodata", ELF::SHT_PROGBITS, ELF::SHF_ALLOC);
+    getStreamer().SwitchSection(ReadOnlySection);
+    getStreamer().EmitIntValue(HiImmOp64, 4);
+    getStreamer().EmitIntValue(LoImmOp64, 4);
+    getStreamer().SwitchSection(CS);
----------------
Nm.


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2874-2875
+
+    TOut.emitRRX(Mips::LW, FirstReg + 1, ATReg,
+                 MCOperand::createExpr(FixupAddr), IDLoc, STI);
+
----------------
Rather than using FirstReg + 1, instead write a helper function to compute the next register. It is unsafe to rely on tablegen ordering the registers in the expected manner.

Currently with FirstReg == Mips::A3, FirstReg + 1 is Mips::AC0, not Mips::T0. 


================
Comment at: lib/Target/Mips/MCTargetDesc/MipsMCCodeEmitter.cpp:714
       break;
+
     case MipsMCExpr::MEK_LO: {
----------------
Spurious newline.


================
Comment at: lib/Target/Mips/MipsInstrFPU.td:606-608
+def LoadImmSingleFGR : MipsAsmPseudoInst<(outs StrictlyFGR32Opnd:$rd),
+                                         (ins imm64:$fpimm),
+                                         "li.s\t$rd, $fpimm">;
----------------
This needs a HARDFLOAT predicate.


================
Comment at: test/MC/Mips/macro-li.d.s:268
+
+li.d	$f4, 1.5 
+# CHECK-MIPS32:   lui     $1, 16376                   # encoding: [0xf8,0x3f,0x01,0x3c]
----------------
Spurious whitespace at the end of this line.


================
Comment at: test/MC/Mips/macro-li.s.s:32
+
+li.s	$4, 1.5 
+# CHECK:   lui     $4, 16320                   # encoding: [0xc0,0x3f,0x04,0x3c]
----------------
Spurious whitespace at the end of this line.


================
Comment at: test/MC/Mips/macro-li.s.s:86
+
+li.s	$f4, 1.5 
+# CHECK:   lui     $1, 16320                   # encoding: [0xc0,0x3f,0x01,0x3c]
----------------
Spurious whitespace at the end of this line.


https://reviews.llvm.org/D14390