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

[Simon Dardis via Phabricator via llvm-commits]

sdardis accepted this revision.
sdardis added a comment.

LGTM with the fixmes added and the whitespace nit addressed.



================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:3088
+
+    if(isABI_O32() || isABI_N32()) {
+      TOut.emitRX(Mips::LUi, ATReg, MCOperand::createExpr(HiExpr), IDLoc, STI);
----------------
Sorry, I missed something here. For O32 and N32 all addresses are 32 bits. For N64 addresses are 64 bits unless -msym32 is used--in which case symbols are 32 bits.

Unfortunately, I've found something questionable in the GNU assembler which is that li.d always assumes that the address of the temporary symbol used to load the constant is always 32 bits in the non-pic case.

Can you attach two fixmes here? The first is that our assembler is technically correct but gives a different result to gas but gas is incomplete there (it has a fixme noting it doesn't work with 64-bit addresses). The second is that with -msym32 the address expansion for N64 should probably use the O32 / N32 case. It's safe to use the 64 address expansion as the symbol's value is considered sign extended.


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:3225-3226
+    getStreamer().SwitchSection(CS);
+
+    if(emitPartialAddress(TOut, IDLoc, Sym))
+      return true;
----------------
FIXME: This method is too general. In principal we should compute the number of instructions required to synthesize the immediate inline compared to synthesising the address inline and relying on non .text sections. For static O32 and N32 this may yield a small benefit, for static N64 this is likely to yield a much larger benefit as we have to synthesize a 64bit address to load a 64 bit value.


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


https://reviews.llvm.org/D14390