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

Stefan Pintilie via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon May 27 10:38:04 PDT 2019

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
+  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();
   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?

  rG LLVM Github Monorepo



More information about the llvm-commits mailing list