[PATCH] D71144: [AIX] Use csect reference for function address constants

[Digger via Phabricator via llvm-commits]

DiggerLin marked 10 inline comments as done.
DiggerLin added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1762
+  if (const GlobalObject *GO = dyn_cast<GlobalObject>(CV)) {
+    if (isa<Function>(GO)) {
+      MCSymbolXCOFF *GOSym = cast<MCSymbolXCOFF>(getSymbol(GO));
----------------
Xiangling_L wrote:
> For the testcase you listed in the summary, for the second part: 
> 
> ```
> void bar() {
> return ;
> }
> void (*bar_ptr1)() = &bar;
> ```
> 
> I think:
> 
> ```
> bar_ptr1:
> .long bar
> ```
> 
> is also a correct output, since function `bar` has definition in current TU, so `bar` here refers to the label within `.csect bar{DS}`. I am okay with either `.long bar` or `.long bar{DS}`. But either way, I think you probably should update the summary to avoid confusion.
agree thanks


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1767
+            TargetLoweringObjectFileXCOFF::getStorageClassForGlobal(GO);
+        GOSym->setStorageClass(SC);
+        MCSectionXCOFF *Csect = OutStreamer->getContext().getXCOFFSection(
----------------
Xiangling_L wrote:
> Since this is the case where one MCSectionXCOFF has only one MCSymbolXCOFF, when we want to get `SC`, we can do `MCSymbolXCOFF->getContainingCsect()`? (Please correct me if I am wrong) If so, do we still want to bother to set `SC` for MCSymbolXCOFF in this case?
we will output GOSym into symboltable in the XCOFFObjectWriter::writeSymbolTableEntryForCsectMemberLabel()
it get the storageclass directly 
 W.write<uint8_t>(SymbolRef.getStorageClass()); 


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1761
+const MCExpr *PPCAIXAsmPrinter::lowerConstant(const Constant *CV) {
+  if (const GlobalObject *GO = dyn_cast<GlobalObject>(CV)) {
+    if (isa<Function>(GO)) {
----------------
hubert.reinterpretcast wrote:
> Shouldn't this just be:
> ```
> if (const Function *F = dyn_cast<Function>(CV)) {
> ```
> ?
good idea, change as suggestion.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1778
+  }
+  return AsmPrinter::lowerConstant(CV);
+}
----------------
hubert.reinterpretcast wrote:
> I suggest to name the direct base class:
> ```
>   return PPCAsmPrinter ::lowerConstant(CV);
> ```
> 
changed  as suggestion


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-FuntionDS.ll:1
+; RUN: llc -verify-machineinstrs -mcpu=pwr7 -mtriple powerpc-ibm-aix-xcoff < %s | FileCheck %s
+
----------------
hubert.reinterpretcast wrote:
> The filename is misspelled. The test name also seems to be the only use of PascalCase of the `aix*` tests in the directory. Additionally, there's also a test in the directory (`aix-func-dsc-gen.ll`) that encodes "function descriptor" differently in its name.
changed file names


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71144