[PATCH] D61694: Boilerplate for producing XCOFF object files from the PowerPC backend.

[Stefan Pintilie via Phabricator via llvm-commits]

stefanp added a comment.

I'm not super familiar with the MC layer but overall this looks good to me.
I have a couple of minor comments.

Also, it seems that there is a part of that patch that is missing. The test added: `aix-xcoff-basic.ll` causes an assert with top of trunk.



================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCAsmInfo.h:41
+public:
+  explicit PPCXCOFFMCAsmInfo();
+};
----------------
Why don't we pass in the `is64Bit` flag and the `Triple` into the function?
I know that we don't intend to support 64 bit from the start but I assume we will want to support it at one point right? We can check the flag at the start and if it is 64 bit we can emit a fatal_error or unreachable as you have done in other places.


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp:87
+  else
+    MAI = new PPCXCOFFMCAsmInfo();
 
----------------
So, previously the default was ELF now it is XCOFF.
This probably won't make much of a difference but ideally we should be spelling out what we support.

```
if (TheTriple.isOSDarwin())
  MAI = new PPCMCAsmInfoDarwin(isPPC64, TheTriple);
else if (TheTriple.isOSBinFormatELF())
   MAI = new PPCELFMCAsmInfo(isPPC64, TheTriple);
else if (TheTriple.isOSBinFormatXCOFF())
   MAI = new PPCXCOFFMCAsmInfo();
else
   llvm_unreachable("Unknown binary format.");
```
However, I don't know if the above is safe. Some code may rely on having ELF as default. Either way, I don't think we can leave it as XCOFF default.


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.h:59
+/// Construct a PPC XCOFF object writer.
+std::unique_ptr<MCObjectTargetWriter> createPPCXCOFFObjectWriter();
+
----------------
Here too maybe we want to keep the 64 bit flag?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61694