[PATCH] D101178: [XCOFF][AIX] Add Global Variables Directly to TOC for 32 bit AIX

[Sean Fertile via Phabricator via llvm-commits]

sfertile added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:468
+
+  assert(!GVType->isVectorTy() &&
+         "A GlobalVariable of Vector type is not currently supported by the "
----------------
Minor nit: since most of these limitations are because this feature is a work in progress, we should change the asserts to `report_fatal_error`. The assert that the type is sized can stay an assert because we expect that to stay a limitation, and because I would expect the user facing option implementation  to have already checked that and emitted some kind of error from the front-end.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:484
+
+  assert(!(GV->hasCommonLinkage() || GV->hasLocalLinkage() ||
+           GV->hasPrivateLinkage()) &&
----------------
I would split out 'hasCommonLinkage()` into its own assert with the string saying something along the lines of `tentative definitions cannot have mapping class XMC_TD`. 

The local linkage limitation I believe we hit when compiling spec right? In that case maybe change the message to 'local linkage globals cannot have mapping class XMC_TD`. If we didn't, then make sure to test with the system assembler first to make sure it is invalid to define a local linkage global in the .toc.


================
Comment at: llvm/test/CodeGen/PowerPC/basic-toc-data-2.ll:1
+; RUN: llc -mtriple powerpc-ibm-aix-xcoff -verify-machineinstrs < %s | FileCheck %s
+
----------------
Minor nit: name this file `basic-toc-data-extern.ll`


================
Comment at: llvm/test/CodeGen/PowerPC/basic-toc-data-2.ll:2
+; RUN: llc -mtriple powerpc-ibm-aix-xcoff -verify-machineinstrs < %s | FileCheck %s
+
+ at i = external global i32, align 4  #0
----------------
Since this patch is only going to add assembly support for this feature, we need to also compile this test to an object file and make sure we fail gracefully.  Add a run-step:

```
; RUN: not --crash llc -filetype=obj -mtriple powerpc-ibm-aix-xcoff  \
; RUN:                 -verify-machineinstrs < %s 2>&1 | \
; RUN:   FileCheck %s --check-prefix=OBJ
```

with a check: `; OBJ: LLVM ERROR:  toc-data not yet supported when writing object files.`

And in  `llvm/lib/MC/XCOFFObjectWriter.cpp` add
```
@@ -439,6 +441,10 @@ void XCOFFObjectWriter::recordRelocation(MCAssembler &Asm,
       TargetObjectWriter->getRelocTypeAndSignSize(Target, Fixup, IsPCRel);
 
   const MCSectionXCOFF *SymASec = getContainingCsect(cast<MCSymbolXCOFF>(SymA));
+
+  if (SymASec->isCsect() && SymASec->getMappingClass() == XCOFF::XMC_TD)
+    report_fatal_error("toc-data not yet supported when writing object files.");
+
   assert(SectionMap.find(SymASec) != SectionMap.end() &&
          "Expected containing csect to exist in map.");
```

to emit the error.


================
Comment at: llvm/test/CodeGen/PowerPC/basic-toc-data.ll:1
+; RUN: llc -mtriple powerpc-ibm-aix-xcoff  -verify-machineinstrs < %s | FileCheck %s
+
----------------
minor nit: name this file `basic-toc-data-def.ll`


================
Comment at: llvm/test/CodeGen/PowerPC/basic-toc-data.ll:2
+; RUN: llc -mtriple powerpc-ibm-aix-xcoff  -verify-machineinstrs < %s | FileCheck %s
+
+ at i = global i32 55, align 4 #0
----------------
We need to similarly fail gracefully for this test when editing an object file, and add test coverage that we do so.

Run-step:
```
; RUN: not --crash llc -filetype=obj -mtriple powerpc-ibm-aix-xcoff  \
; RUN:                 -verify-machineinstrs < %s 2>&1 | \
; RUN:   FileCheck %s --check-prefix=OBJ
```

and to get the error emitted, add a switch case for `XMC_TD:` in `XCOFFObjectWriter::getCsectGroup` which emits the error. 


================
Comment at: llvm/test/CodeGen/PowerPC/toc-data.ll:21
+; TEST:         .write_int:
+; TEST:         la 4, i[TD](2)
+; TEST-NEXT:    stw 3, 0(4)
----------------
minor nit: indent the instructions 2 spaces past the label, ditto for the other checks below.


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

https://reviews.llvm.org/D101178