[PATCH] D41949: [RISCV] [WIP] implement li pseudo instruction

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

Hi Mario - as this is marked in WIP I've added a few initial comments rather than given a 100% complete review.

It's a shame we need to have RISCVInstrInfo::movImm32 as well as the expansion introduced here - but of course one produces MachineInstr and the other produces MCInstr.



================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:979-981
+    int64_t high = ((ImmOp.getImm() + 0x800) >> 12) & 0xFFFFF;
+    int64_t low = SignExtend64<12>(ImmOp.getImm() & 0xFFF);
+    if (high && low) {
----------------
Locals are normally capitalised in the LLVM coding style.


================
Comment at: lib/Target/RISCV/RISCVInstrFormats.td:108-109
   let isPseudo = 1;
-  let isCodeGenOnly = 1;
+  let isCodeGenOnly = !eq(opcodestr,"");
+  let isAsmParserOnly = !eq(opcodestr,"");
 }
----------------
Do you think having these properties inferred might be a big 'magic'? I'm not really decided one way or another myself, but it does seem a bit non-obvious.


================
Comment at: test/MC/RISCV/rvi-aliases-valid.s:33
+li x10, 0
+# CHECK: addi a0, zero, 2047
+li x10, 2047
----------------
Having CHECK-INST and CHECK-ALIAS makes sense in this file. Adding in CHECK to the mix makes it a little confusing. Maybe the li cases that don't 'round-trip' belong in a separate test file? e.g. to match gnu as behaviour we'd expect `li x3, 0x80` to be printed by objdump, but ` li x4, 0x800` would be expanded to lui+addiw and will never appear in objdump output.


https://reviews.llvm.org/D41949