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

[Mario Werner via Phabricator via llvm-commits]

niosHD added a comment.

In https://reviews.llvm.org/D41949#1067297, @asb wrote:

> I'd do the compressed changes in a different patch. Thanks for updating the peephole RISCVISelDAGToDAG, I'll review that bit ASAP and then commit. At a first look, it seems to handle this exactly as I would expect.


Agreed! I will add compression in a different patch. Considering the inline discussion with Ana and Sameer, any opinion on what is the cleanest way to add compression?



================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVMCPseudoExpansion.cpp:44
+    if (Hi20) {
+      Out.EmitInstruction(
+          MCInstBuilder(RISCV::LUI).addReg(DestReg).addImm(Hi20), *STI);
----------------
sabuasal wrote:
> Hi Mario @niosHD  ,
> 
> Thanks for the patch, this looks nice.
> 
>  just a small note about your comment addressing compression in case you want to update it in the future like @asb suggested.
> 
> Since you are calling your function (emitRISCVLoadImm) from the InstPrinter (RISCVAsmPrinter::EmitInstruction)  the standard way to Emit the Instructoin is by calling EmitToStreamer in your AsmPrinter. 
> 
> In other back-ends this will call ( AsmPrinter::EmitToStreamer), In RISCV, we define our own EmitToStreamer all what you have to do to support compression is calling your AsmPrinter->EmitToStreamer().
> 
> 
Hi Sameer @sabuasal,

thank you for the hint but I do not think that calling `AsmPrinter::EmitToStreamer` is easily possible. `emitRISCVLoadImm` takes an `MCStreamer` as input because it is available in both, the `RISCVAsmParser` and the `RISCVAsmPrinter`, where it is called from.

My current compression prototype therefore simple adds the same compression code that has been added to the `RISCVAsmParser` and the `RISCVAsmPrinter` to `emitRISCVLoadImm` (via a static helper function in `RISCVMCPseudoExpansion.cpp`). However, I am not particularly fond of this duplication and am open for alternative ideas.


================
Comment at: lib/Target/RISCV/RISCVAsmPrinter.cpp:74
+    const MachineOperand &ImmOp = MI->getOperand(1);
+    emitRISCVLoadImm(DstRegOp.getReg(), ImmOp.getImm(), *OutStreamer,
+                     &getSubtargetInfo());
----------------
sabuasal wrote:
> niosHD wrote:
> > apazos wrote:
> > > can't we return the new instruction from this function and reuse the EmitToStreamer call below.  This way we reduce the places to insert compression calls, when instruction compression at MC level is enabled.
> > Theoretically yes, but isn't compression done in the `EmitInstruction` of the streamer?
> > 
> > The code here is basically a custom MI to MC lowering. It uses the same `EmitInstruction` function which is also used by the generated `emitPseudoExpansionLowering` internally. Maybe I miss something but assuming that the MC compression works in conjunction with pseudo expansion I expect that it also works for the current code.
> > 
> I believe I addressed this in my other comment but I actually just saw this comment you had!
> 
> 
> The way "emitPseudoExpansionLowering" emits the instruction is  "EmitToStreamer(OutStreamer, TmpInst);". This way it preserves any behavior in the XXXASMPrinter it is called from.   You can check that in any inc file "XXXXGenMCPseudoLowering.inc"
(see above)

Returning the instructions, as Ana suggested in the first comment, would be an alternative to adding compression to the `RISCVMCPseudoExpansion`. However, I am still not sure if it is idiomatic for the llvm code base to return a list of instructions from such a function. Further opinions are welcome!


https://reviews.llvm.org/D41949