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

[Sean Fertile via Phabricator via llvm-commits]

sfertile added a comment.

Thanks for the quick updates. Patch is getting really close I think.



================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:2185
+
+  // Handle XCOFF::TD case first, then deal with the rest
+  if (const GlobalVariable *GVar = dyn_cast<GlobalVariable>(GO))
----------------
Real minor nit: missing the period at the end of the comment.


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:2229
     const GlobalObject *GO, SectionKind Kind, const TargetMachine &TM) const {
+  // Handle XCOFF::TD case first, then deal with the rest
+  if (const GlobalVariable *GVar = dyn_cast<GlobalVariable>(GO))
----------------
ditto.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:854
+
+    // Transform %rN = ADDItoc @op1, %r2
+    LowerPPCMachineInstrToMCInst(MI, TmpInst, *this);
----------------
Missing the period.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:857
+
+    // Change the opcode to la
+    TmpInst.setOpcode(PPC::LA);
----------------
Real minor nit: Probably useful to use 'load address' instead of 'la', and missing the period at the end of the sentance.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:449
+
+  // TODO: assert on alignment and size, etc. Check that the GlobalVariable
+  // meets the criteria to be added to the TOC (TBD)
----------------
I'm not sure of the limitations of the option in XL that enables toc-data. For example if it can it be used to put a vector with an alignment/size of 16 in the TOC, or structures,  arrays, etc; For now we don't have any testing coverage for these things so I think we should add some defensive errors here.

* If the alignment is greater then the alignment of a toc-entry it should be an error.
* If the size of the type is larger then a toc-entry it should be an error.
* For now, if the GlobalVariable is an array type or aggregate type we should error. 


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

https://reviews.llvm.org/D101178