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

[Luo Jia via Phabricator via llvm-commits]

luojia added a comment.

In D145539#4176791 <https://reviews.llvm.org/D145539#4176791>, @jrtc27 wrote:

> 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...

Yes llvm-objcopy can do this and this is the current approach on EDK2 RISC-V. But for languages like Rust it will be more convenient if the compiler backend writes it out directly from build target configuration, or it will require following building steps (in this case, a objcopy step) where complex build frameworks like `xtask` would have to be used; it would also sometimes result in difference of output file paths where automatic tests (e.g. build a list of UEFI apps to test UEFI implementation) would be complex to develop.

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

Thanks! I'll do that.



================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp:670
+class COFFRISCVAsmBackend : public RISCVAsmBackend {
+  const Triple &TheTriple;
+
----------------
jrtc27 wrote:
> Isn't this available from the parent class's STI?
Thanks! I'll make STI protected in super class and use STI.getTargetTriple() on createObjectTargetWriter function.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.h:97
 };
-}
+} // namespace llvm
 
----------------
jrtc27 wrote:
> Unnecessary churn
Thanks! Will remove.


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


================
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
----------------
jrtc27 wrote:
> Don't think this adds value... the backend would need extensive changes for RV128
Yes. It's a note that future RV128 developers may need to change this part when they are adapting LLVM to 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.
----------------
jrtc27 wrote:
> 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
I'll modify comments to show that this is unlikely to happen.


================
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");
----------------
jrtc27 wrote:
> This doesn't print its error message in NDEBUG builds, it's just __builtin_unreachable. I think you want report_fatal_error.
Changed to report_fatal_error.


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

https://reviews.llvm.org/D145539