[PATCH] D66154: [PowerPC][AIX] Adds support for writing the .data section in assembly files

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:165
 /// and legal.
-static unsigned getGVAlignmentLog2(const GlobalValue *GV, const DataLayout &DL,
-                                   unsigned InBits = 0) {
+unsigned AsmPrinter::getGVAlignmentLog2(const GlobalValue *GV,
+                                        const DataLayout &DL,
----------------
This does not need to be a non-static member function. It can be a static member function.


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:1122
                                          unsigned MaxBytesToEmit) {
+  if (MAI->useDotAlignForPowerOfTwoAlignment()) {
+    // Only power of two alignments are supported with .align
----------------
Given the code here, the name of the function (and the variable) is misleading. It should say `useDotAlignForAlignment`, because we don't fall back to anything else when the requested alignment is not a power of two.


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:1123
+  if (MAI->useDotAlignForPowerOfTwoAlignment()) {
+    // Only power of two alignments are supported with .align
+    if (!isPowerOf2_32(ByteAlignment))
----------------
With the suggestion to change the error text below, I think we can remove the comment.


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:1125
+    if (!isPowerOf2_32(ByteAlignment))
+      report_fatal_error("Non-power of two alignments are unsupported.");
+
----------------
Suggestion: "Only power-of-two alignments are supported with .align."


================
Comment at: llvm/lib/MC/MCSectionXCOFF.cpp:33
+    if (getMappingClass() != XCOFF::XMC_RW)
+      llvm_unreachable("Unsupported storage-mapping class for .data csect");
+
----------------
What we should write here, and whether `llvm_unreachable` is appropriate depends. I think that the current patch allows us to say that we do not handle switching to data sections with other storage mapping classes yet. That can be an assert, but probably not an unreachable.

```
      assert(getMappingClass() == XCOFF::XMC_RW &&
             "Unhandled storage-mapping class for data section.");
```


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1662
   SectionKind GVKind = getObjFileLowering().getKindForGlobal(GV, TM);
-  if (!GVKind.isCommon() && !GVKind.isBSSLocal())
-    report_fatal_error("Only common variables are supported on AIX for now.");
+  if (!GVKind.isCommon() && !GVKind.isBSS() && !GVKind.isData())
+    report_fatal_error("Unsupported global variable kind.");
----------------
Stray change from `isBSSLocal` to `isBSS`. Please fix.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1663
+  if (!GVKind.isCommon() && !GVKind.isBSS() && !GVKind.isData())
+    report_fatal_error("Unsupported global variable kind.");
 
----------------
Suggestion: "Encountered a global variable kind that is not supported yet."


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-data.ll:25
+; CHECK:      .globl  svar
+; CHECK-NEXT: .align  1
+; CHECK-NEXT: svar:
----------------
We should seek to understand why GCC and XL uses 4-byte alignment.


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

https://reviews.llvm.org/D66154