[PATCH] D41932: [RISCV] Hooks for enabling instruction compression

[Sameer AbuAsal via Phabricator via llvm-commits]

sabuasal added inline comments.


================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp:52
+  MCInst CInst;
+  bool Res = compressInst(CInst, Inst, STI, getContext());
+  MCELFStreamer::EmitInstruction((Res ? CInst : Inst), STI);
----------------
asb wrote:
> sabuasal wrote:
> > asb wrote:
> > > efriedma wrote:
> > > > I'm not sure it makes sense to do compression here.  Software using the MC layer should have full control over what instructions it's emitting, where possible.  And to get the right output with "-riscv-no-aliases", you have to run compression before you output assembly.
> > > > 
> > > > If the backend is okay delaying compression until really late, maybe you could call it in LowerRISCVMachineInstrToMCInst() or something like that?
> > > It's important that we reuse the same MCInst->MCInst mapping both for the codegen and assembly/disassembly path, but you're right - it seems like it would be viable to call this in LowerRISCVMachineInstrToMCInst - doing the usual lowering and then trying to transform `OutMI`. Do you see any issues with that Ana/Sameer?
> > @efriedma @asb 
> > 
> > "LowerRISCVMachineInstrToMCInst" Is not on the path for the Assembler\Disassembler (llvm-mc). Adding compression logic there will result in llvm-mc not compressing instructions passed from an ASM file.  The reason compression changes where added to the streamer is because it is the onlt location where all paths (Assembler. MI lowering) meet.
> I recognise LowerRISCVMachineInstrToMCInst won't be called on the llvm-mc path, but this patch does add calls to the asm parser and instprinter, which at first glance seems sufficient for the llvm-mc case. Is that not so? It's been a while since I looked at this part of the instruction compression discussion, so I might be missing/forgetting something.
> 
> Updating this diff with a patch summary briefly explaining the locations compressInst is called would be helpful.
I added a summary for why every hook was added to the patch.

For the llvm-mc and llvm-objdump path having the hooks in AsmParser and in Instr Printer is sufficient for what we are trying to achieve.

The hook to the Elfstreamer was added for the path on 1.c ---> 1.o. This could be moved earlier like @efriedma  suggested (I tried it by calling compression in RISCVAsmPrinter::EmitInstruction right after  it calls LowerRISCVMachineInstrToMCInst).  This also has another advantage of saving a call to compressInst for the path (1.s ---> 1.o); as is compressInst will be called both at the AsmParster and at the ELfStreamer.

However, the only possible side effect is that we will no longer be matching the gcc fronted in this. going from a (1.c ---> 1.s) gcc never emits a compressed  asm instruction. While clang can do that if we pass -riscv-no-aliases. The gcc equivilent flag for this (-M -no-aliases) only works with gcc-objdump and not the fronted. 

If we don't forsee this as an issue then we can move the hook with no problem.






https://reviews.llvm.org/D41932