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

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

In https://reviews.llvm.org/D41949#974406, @niosHD wrote:

> 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.


Yes, I think using PseudoLI in codegen could make sense, and as you say this could allow the reuse of a common helper function.



================
Comment at: lib/Target/RISCV/RISCVInstrFormats.td:108-109
   let isPseudo = 1;
-  let isCodeGenOnly = 1;
+  let isCodeGenOnly = !eq(opcodestr,"");
+  let isAsmParserOnly = !eq(opcodestr,"");
 }
----------------
niosHD wrote:
> 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.)
> 
Could you just override isCodeGenOnly/isAsmParserOnly when necessary:

```
let isAsmParserOnly = 1 in
def FooInst : Pseudo<....>
```


================
Comment at: test/MC/RISCV/rvi-aliases-valid.s:33
+li x10, 0
+# CHECK: addi a0, zero, 2047
+li x10, 2047
----------------
niosHD wrote:
> 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.
Yes, I see your concern. My main problem is that when there was just CHECK-INST and CHECK-ALIAS it was fairly obvious what the different check lines meant. A comment in the file that explains the different check lines might make it easier on the reader. I suppose `CHECK-EXPAND` might be a little more descriptive, seeing as we're verifying that the pseudoinstruction expands to the expected multi-instruction sequence?


https://reviews.llvm.org/D41949