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

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1767
+            TargetLoweringObjectFileXCOFF::getStorageClassForGlobal(GO);
+        GOSym->setStorageClass(SC);
+        MCSectionXCOFF *Csect = OutStreamer->getContext().getXCOFFSection(
----------------
DiggerLin wrote:
> 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()); 
Thanks for point this out, @DiggerLin. It sounds like this change may introduce a difference how the symbol table entries for the csect and the label are produced. Please look into adding appropriate testing. Note that the status quo in `llvm/test/CodeGen/PowerPC/aix-func-dsc-gen.ll` does not use the same storage class for the csect and the label. Note also that it is not generally okay to use the same storage class for the csect and the label (https://reviews.llvm.org/D71125?id=232583#inline-646944).

I believe @daltenty can elaborate on the refactor he mentions (https://reviews.llvm.org/D71144?id=232631#inline-644323). If I understand correctly, said refactor looks to be unavoidable. If this patch is indeed stuck, please indicate it. I think the "Plan Changes" option in Phabricator is a good way to do so.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-reference-func-addr-const.ll:2
+; RUN: llc -verify-machineinstrs -mcpu=pwr7 -mtriple powerpc-ibm-aix-xcoff < %s | FileCheck %s
+; RUN: llc -verify-machineinstrs -mcpu=pwr7 -mtriple powerpc64-ibm-aix-xcoff < %s | FileCheck --check-prefix=CHECK64 %s
+
----------------
@cebowleratibm, we're still wondering the reason behind using `pwr7` here. When do we choose to use `pwr4`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71144