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

[Mario Werner via Phabricator via llvm-commits]

niosHD added a comment.

Hi Alex, thank you for your comments!

As mentioned two weeks ago, I also think that it would be nice if we can share the code that synthesizes immediates between the assembler and codegen. I plan to experiment with getting this working in the next week. The idea that I plan to investigate is to delay the generation of the immediates until the MC layer is reached.  Basically, emitting `PseudoLI` machine instructions in `RISCVInstrInfo::movImm32` and perform the actual expansion manually during MI to MC lowering.



================
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) {
----------------
asb wrote:
> Locals are normally capitalised in the LLVM coding style.
Right, I really need read the coding style again. Will be fixed in the next iteration.


================
Comment at: lib/Target/RISCV/RISCVInstrFormats.td:108-109
   let isPseudo = 1;
-  let isCodeGenOnly = 1;
+  let isCodeGenOnly = !eq(opcodestr,"");
+  let isAsmParserOnly = !eq(opcodestr,"");
 }
----------------
asb wrote:
> 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.
Well, the motivation for the magic was simply to keep the patch minimal. I at first intended to introduce a new `AsmPseudo` class. However, I decided against it because deriving from Pseudo did not feel particularly clean given that a real `AsmPseudo` is typically not a `CodeGenPseudo`.  

With the mental model that a Pseudo is either a `AsmPseudo` or  a `CodeGenPseudo`, inferring the type depending on  opcodestr is not that bad. Anyway, I got the code wrong. The `isAsmParserOnly` assignment should be negated (i.e., `!if(!eq(opcodestr,""), 0, 1)`).

Regarding alternatives, for me, the cleanest option would be to introduce a new `PseudoBaseClass` and derive a `AsmPseudo` and `CodeGenPseudo` from it. (Naming suggestions are welcome.)



================
Comment at: test/MC/RISCV/rvi-aliases-valid.s:33
+li x10, 0
+# CHECK: addi a0, zero, 2047
+li x10, 2047
----------------
asb wrote:
> 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.
If you are not strongly against it I would prefer to keep all pseudo instruction test cases together in the respective `...aliases-valid` and `...aliases-invalid` files. We already have many files in the RISCV MC test directory and I am hesitant to add even more without real need. I expect that the remaining pseudo instructions most likely will not properly roundtrip either and certainly do not want to add new test files for every individual instruction.

Also, as you noted, some `li` instructions will roundtrip depending on the specified immediate. Splitting the files based on this property would introduce even more test files given that RV32 and RV64 need separate tests too.


https://reviews.llvm.org/D41949