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

[Mario Werner via Phabricator via llvm-commits]

niosHD added a comment.

Thank you Ana for your comments!



================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:601
   case Match_Success:
-    Inst.setLoc(IDLoc);
-    Out.EmitInstruction(Inst, getSTI());
-    return false;
+    return processInstruction(Inst, IDLoc, Out, STI);
   case Match_MissingFeature:
----------------
apazos wrote:
> You changed getSTI() -> STI, was it intentional?
No, good catch, although I am not sure if it is better to use `getSTI` compared to directly accessing STI. (Probably a matter of taste.)

On a closer look, actually, the whole `MCSubtargetInfo` operand of `processInstruction` seems to be redundant and can be removed given that we can access the STI within the method as well.

Still, is it preferred to access `STI` via `getSTI` in the AsmParser?



================
Comment at: lib/Target/RISCV/RISCVAsmPrinter.cpp:74
+    const MachineOperand &ImmOp = MI->getOperand(1);
+    emitRISCVLoadImm(DstRegOp.getReg(), ImmOp.getImm(), *OutStreamer,
+                     &getSubtargetInfo());
----------------
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.



https://reviews.llvm.org/D41949