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

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/lib/MC/MCObjectFileInfo.cpp:769
+  TextSection = Ctx->getXCOFFSection(
+      ".text", XCOFF::StorageMappingClass::XMC_PR, SectionKind::getText());
+}
----------------
This choice of csect name is not a fundamental property of the object file format or of the ABI. A comment with suitable justification should be added.

I believe a good justification is that, on AIX, the `__section__` attribute is not implemented by the IBM XL compiler, and the choice of using ".text" is compatible with GCC.


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp:249
+  void emitMachine(StringRef CPU) override {
+    llvm_unreachable("emit machine pseduo-op no tsupported on XCOFF.");
+  }
----------------
hubert.reinterpretcast wrote:
> Typos. Suggestion (if indeed the restriction is inherent):
> "Invalid request to emit a machine pseudo-op for XCOFF."
> 
Suggestion (to avoid implying that there are valid machine pseudo-ops for XCOFF, should someone encounter the message): "Machine pseudo-ops are invalid for XCOFF."


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp:253
+  void emitAbiVersion(int AbiVersion) override {
+    llvm_unreachable("Invalid machine pseudo-op for XCOFF.");
+  }
----------------
Copy/paste error. Suggestion: "ABI-version pseudo-ops are invalid for XCOFF."


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp:257
+  void emitLocalEntry(MCSymbolELF *S, const MCExpr *LocalOffset) override {
+    llvm_unreachable("Invalid machine pseudo-op for XCOFF.");
+  }
----------------
Copy/paste error. Suggestion: "Local-entry pseudo-ops are invalid for XCOFF."


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-basic.ll:7
+
+; RUN: llc -mtriple powerpc-ibm-aix-xcoff  < %s | \
+; RUN: FileCheck --check-prefix=ASM %s
----------------
The other RUN lines do not have `  < %s` with two consecutive spaces.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-basic.ll:31
+
+; ASM: .csect .text[PR]
----------------
I would prefer to have a comment to the effect that the csect need not be present, but we generate it anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61694