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

Simon Dardis via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Feb 15 09:13:48 PST 2017


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

A few small things. Mostly the changes necessary are along the lines of differentiating between PIC and non-PIC. I've written them inline. The other minor change is that you should remove the stub implementation of the literal section and submit that afterwards. It will be easier to implement/review then.



================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2980-2986
+    uint32_t 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;
----------------
Rather than going through void pointers, use BitsToDouble & co. from llvm/Support/MathExtras.h.


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:3021-3038
+      const MipsMCExpr *LitExpr =
+          MipsMCExpr::create(MipsMCExpr::MEK_LITERAL, LitSym, getContext());
+
+      int32_t size_data_fragment = 0;
+      MCSection::FragmentListType &FragList =
+          ReadOnlySection->getFragmentList();
+      if (!FragList.empty()) {
----------------
This section isn't quite right.

In the PIC case, we have to load the address of the constant via the GOT, with a R_MIPS_GOT16 relocation attached to an lw. Then load the constant with ldc with a relocation type R_MIPS_LO16 attached to it.

In the non-pic case, it's lui with a R_MIPS_HI16 relocation, then a ldc1 with a R_MIPS_LO16.


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:3052-3057
+      if (loadImmediate(HiImmOp64, FirstReg, Mips::NoRegister, true, true,
+                        IDLoc, Out, STI))
+        return true;
+
+      if (loadImmediate(0, nextReg(FirstReg), Mips::NoRegister, true, true,
+                        IDLoc, Out, STI))
----------------
This is the expected behaviour for loading a 64bit value in 32bit GPRs. For 64 bit GPRs we have to load  it into a single register. Test the ABI to determine if we should load into one or two registers. 


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:3081-3107
+    const MCExpr *HiSym =
+        MCSymbolRefExpr::create(Sym, MCSymbolRefExpr::VK_None, getContext());
+
+    const MipsMCExpr *HiExpr =
+        MipsMCExpr::create(MipsMCExpr::MEK_HI, HiSym, getContext());
+
+    const MCExpr *LoSym =
----------------
This hunk needs to be re-arranged slightly. The first step is to generate an emit the upper portion of the constant's address for non-PIC or load the address of the the constant from the GOT. Both cases set up $at.

The second step is to build an expression that refers to the base address of the constant.

Then finally, if the ABI is N32 or N64 perform emit ld, otherwise since it's O32, emit two loads.


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:3121
+        return true;
+      if (hasMips64())
+        TOut.emitRR(Mips::DMTC1, FirstReg, ATReg, IDLoc, STI);
----------------
This should be

  if (isABI_N32() || isABI_N64())

as the instruction expansion is dependant on the ABI.


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:3151-3167
+    MCSymbol *Sym = getContext().getOrCreateSymbol(
+        (dyn_cast<MCSectionELF>(ReadOnlySection))->getSectionName());
+
+    const MCExpr *LitSym =
+        MCSymbolRefExpr::create(Sym, MCSymbolRefExpr::VK_None, getContext());
+
+    const MipsMCExpr *LitExpr =
----------------
This section isn't quite right.

In the PIC case, we have to load the address of the constant via the GOT, with a R_MIPS_GOT16 relocation attached to an ld. Then load the constant with ldc with a relocation type R_MIPS_LO16 attached to it.

In the non-pic case, it's lui with a R_MIPS_HI16 relocation, then a ldc1 with a R_MIPS_LO16.


================
Comment at: test/MC/Mips/macro-li.s:1
-# RUN: llvm-mc %s -triple=mips-unknown-linux -show-encoding -mcpu=mips32r2 | \
-# RUN:   FileCheck %s
----------------
This file can be kept, it's testing a different macro.


https://reviews.llvm.org/D14390





More information about the llvm-commits mailing list