[PATCH] D145539: [llvm/Target] Add Windows COFF support for RISC-V

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added a comment.

I'm guessing this is so you can llvm-objcopy an ELF into a PE/COFF?.. Because without relocations being defined you can't do much else of use...

And please upload diffs with full context as per https://llvm.org/docs/DeveloperPolicy.html#making-and-submitting-a-patch



================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp:670
+class COFFRISCVAsmBackend : public RISCVAsmBackend {
+  const Triple &TheTriple;
+
----------------
Isn't this available from the parent class's STI?


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp:688
   const Triple &TT = STI.getTargetTriple();
   uint8_t OSABI = MCELFObjectTargetWriter::getOSABI(TT.getOS());
+
----------------
Can be after the if


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.h:97
 };
-}
+} // namespace llvm
 
----------------
Unnecessary churn


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVWinCOFFObjectWriter.cpp:32
+
+unsigned RISCVWinCOFFMachineFromTriple(const Triple &TheTriple) {
+  if (TheTriple.isRISCV32()) {
----------------
static


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVWinCOFFObjectWriter.cpp:38
+  } else {
+    // FIXME: once TheTriple.isRISCV128() is implemented,
+    // map it to COFF::IMAGE_FILE_MACHINE_RISCV128
----------------
Don't think this adds value... the backend would need extensive changes for RV128


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVWinCOFFObjectWriter.cpp:52
+                                                const MCAsmBackend &MAB) const {
+  // FIXME: RISCV does not have Windows COFF relocation type definitions yet;
+  // add a RISCVWinCOFFObjectWriter function here once those types are declared.
----------------
This isn't actionable unless Microsoft gain an interest in Windows on RISC-V, so I don't think this is something we want to have a comment for as it's not possible to fix


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVWinCOFFObjectWriter.cpp:54
+  // add a RISCVWinCOFFObjectWriter function here once those types are declared.
+  llvm_unreachable(
+      "RISCV does not have Windows COFF relocation type definitions yet");
----------------
This doesn't print its error message in NDEBUG builds, it's just __builtin_unreachable. I think you want report_fatal_error.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145539/new/

https://reviews.llvm.org/D145539