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

[Alex Bradbury via Phabricator via llvm-commits]

asb added a subscriber: llvm-commits.
asb added a comment.

Adding llvm-commits as a subscriber. A couple more comments inline



================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp:52
+  MCInst CInst;
+  bool Res = compressInst(CInst, Inst, STI, getContext());
+  MCELFStreamer::EmitInstruction((Res ? CInst : Inst), STI);
----------------
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.


================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp:53
+  bool Res = compressInst(CInst, Inst, STI, getContext());
+  MCELFStreamer::EmitInstruction((Res ? CInst : Inst), STI);
+}
----------------
apazos wrote:
> The instruction will only be transformed if compression extension is enabled.
> If the user enables compression, but does not want a particular instruction to be compressed, RISCV has no assembler directive to allow that (does it Alex? Maybe we can request that feature?). As Eli knows ARM supports "adds.w" to allow that kind of  fine grain control.
> We need to also check if LowerRISCVMachineInstrToMCInst is invoked when coming from assembly/disassembly path.
There is no current directive to control automatic compression separately from support for the C extension. If you want to compile an assembly file with support for the C extension but have complete control over when compressed instructions are chosen you currently need to use .option rvc and .option norvc (in gas - not yet implemented in llvm-mc). e.g.


```
.option norvc
addi x1, x1, 1
.option rvc
c.addi x1, 1
```


https://reviews.llvm.org/D41932