[PATCH] D41658: [RISCV] Encode RISCV specific ELF e_flags to RISCV Binary by RISCVTargetStreamer

[Shiva Chen via Phabricator via llvm-commits]

shiva0217 added inline comments.


================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVMCTargetDesc.cpp:73
+createRISCVObjectTargetStreamer(MCStreamer &S, const MCSubtargetInfo &STI) {
+  return new RISCVTargetELFStreamer(S, STI);
+}
----------------
apazos wrote:
> Should'nt we check for ELF format, that is the only one supported now, but may change later? 
> 
> const Triple &TT = STI.getTargetTriple();
>   if (TT.isOSBinFormatELF())
>     return new RISCVTargetELFStreamer(S);
>   return nullptr;
Add ELF format checking before new RISCVTargetELFStreamer.


================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVMCTargetDesc.cpp:85
     TargetRegistry::RegisterMCSubtargetInfo(*T, createRISCVMCSubtargetInfo);
+    TargetRegistry::RegisterObjectTargetStreamer(
+        *T, createRISCVObjectTargetStreamer);
----------------
apazos wrote:
> Reading MCStreamer.h I understand when we define a target streamer we need to define two streamers, ELF and Asm. Is it how you interpreted?
> 
> /// Target specific streamer interface. This is used so that targets can
> /// implement support for target specific assembly directives.
> ///
> /// If target foo wants to use this, it should implement 3 classes:
> /// * FooTargetStreamer : public MCTargetStreamer
> /// * FooTargetAsmStreamer : public FooTargetStreamer
> /// * FooTargetELFStreamer : public FooTargetStreamer
> 
To my understanding, AsmStreamer is used to emit target specific assembly directives,
E.g. ARM emit .fpu directive by AsmStreamer and encode the information to a section by ELFStreamer. It seems that RISCV GCC will not emit directive to specify target feature(E.g. C EXT ISA), so I define ELFStreamer first. I think we could introduce the AsmStreamer in the future patch when we need to emit assembly directives.


================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp:41
+MCELFStreamer &RISCVTargetELFStreamer::getStreamer() {
+  return static_cast<MCELFStreamer &>(Streamer);
+}
----------------
apazos wrote:
> Shouldn't it be return static_cast<RISCVELFStreamer &>(Streamer);
We didn't define RISCVELFStreamer class for emitting e_flags. It seems that we could introduce the class in the feature patch?


Repository:
  rL LLVM

https://reviews.llvm.org/D41658