[AIX] Use csect reference for function address constants

[DiggerLin]

Summary

We currently emit a reference for function address constants as labels;
for example:

foo_ptr:
.long foo
however, there may be no such label in the case where the function is
undefined. Although the label exists when the function is defined, we
will (to be consistent) also use a csect reference in that case.

[Xiangling_L]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1762

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.

1767

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?

llvm/test/CodeGen/PowerPC/aix-xcoff-FuntionDS.ll

1 ↗

(On Diff #232631)

To generate assembly from IR, please add -verify-machineinstrs and cpu level -mcpu=pwr7.

3 ↗

(On Diff #232631)

You can peel off extra useless info like align 4, #0 etc. from testcase to make it cleaner.

[daltenty]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1762

I'd like to continue to use .long bar{DS} to keep consistent with what's in getMCSymbolForTOCPseudoMO, as you noted in the summary.

But I agree you should probably update the summary to clarify it, starting with a description of the problem we trying to solve rather than just the expected output, which can be confusing if you don't already know why we expect what we do.

1772

Aside: This would probably also benefit from the refactor suggested in D71125

llvm/test/CodeGen/PowerPC/aix-xcoff-FuntionDS.ll

2 ↗

(On Diff #232631)

Is there a reason not to check the 64-bit case?

[hubert.reinterpretcast]

The commit subject should be more direct in indicating what the patch does, not what problem it solves.

The message body reads too much like a problem report.

Suggestion:

[AIX] Use csect reference for function address constants

We currently emit a reference for function address constants as labels;
for example:

foo_ptr:
.long foo

however, there may be no such label in the case where the function is
undefined. Although the label exists when the function is defined, we
will (to be consistent) also use a csect reference in that case.

[hubert.reinterpretcast]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1761

Shouldn't this just be:

if (const Function *F = dyn_cast<Function>(CV)) {

?

1778

I suggest to name the direct base class:

return PPCAsmPrinter ::lowerConstant(CV);

1779

Empty line between function bodies please.

llvm/test/CodeGen/PowerPC/aix-xcoff-FuntionDS.ll

1 ↗

(On Diff #232631)

Why pwr7?

[Xiangling_L]

llvm/test/CodeGen/PowerPC/aix-xcoff-FuntionDS.ll

1 ↗

(On Diff #232631)

@cebowleratibm To be honest, I don't know the reason underneath, and I vaguely recall that we decided to set it to -mcpu=pwr7 in the srum?

[hubert.reinterpretcast]

llvm/test/CodeGen/PowerPC/aix-xcoff-FuntionDS.ll

1 ↗

(On Diff #233142)

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.

[DiggerLin]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1761

good idea, change as suggestion.

1762

agree thanks

1767

we will output GOSym into symboltable in the XCOFFObjectWriter::writeSymbolTableEntryForCsectMemberLabel()
it get the storageclass directly
W.write<uint8_t>(SymbolRef.getStorageClass());

1778

changed as suggestion

llvm/test/CodeGen/PowerPC/aix-xcoff-FuntionDS.ll

1 ↗

(On Diff #233142)

changed file names

[hubert.reinterpretcast]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1767

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.

llvm/test/CodeGen/PowerPC/aix-reference-func-addr-const.ll

3

@cebowleratibm, we're still wondering the reason behind using pwr7 here. When do we choose to use pwr4?

[DiggerLin]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1767

the patch is only for the assembly, not for the xcoff objectfile output.

because we do not invoke a getAssembler().registerSection(*Section) and
getAssembler().registerSymbol(*Symbol) in the patch.
it can not output the foo (extern function) symbol entry in the symbol table in xcoff objectfile.
and it also can not output the csect in the symbol table(it is not necessary need csect symbol output here ).

I will create a new patch for to output the foo (extern function) symbol entry in the symbol table in xcoff objectfile later.

And I will change the storage class for SC to HIDDEN here.

[DiggerLin]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1767

sorry , change the storage class from SC to HIDDEN will have problem. since we look external symbol as csect. and we will use writeSymbolTableEntryForControlSection to write the external function symbol

[jasonliu]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1767

@DiggerLin
I don't think we need to generate the GOSym (now the FSym) in the symbol table.
Currently in the XCOFFObjectWriter.cpp, if it's an undefined symbol, we would get the symbol's containingCsect, and generate the csect as symbol. If it's not an undefined symbol, we are referring the csect symbol right now, we are still generating the csect as symbol. We do not have label symbol involved for reference for function address.

@hubert.reinterpretcast @daltenty
Could you elaborate on why you think the refactoring is unavoidable? I'm a bit confusing here. An example would be great (if possible).

[jasonliu]

llvm/test/CodeGen/PowerPC/aix-reference-func-addr-const.ll

3

Discussed offline today, I will try to summarize the discussion.
Setting it to pwr7 makes people wondering about the reason of why we are choosing pwr7 (which is not our base support on AIX). So for consistency, we should just use -mcpu=pwr4, the base cpu we choose to support instead.
For testing that could be cpu level sensitive (ABI for example), we should test both the base, and the cpu level that's in interest.
In the future, if we decide to move up the default cpu level support on AIX, we should modify all the test case that intend to test the default cpu level to that higher new default cpu level.
Also part of the reason we have this discussion right now is that our default cpu level for AIX is not correct right now. (if it is, then we can just omit the -mcpu option all together.) So we should consider fix this issue as well.

[hubert.reinterpretcast]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1767

I don't think we need to generate the GOSym (now the FSym) in the symbol table.

@jasonliu, do you mean that we should stop generating a label for the function descriptor in both the assembly writing and the direct object file writing paths?

sorry , change the storage class from SC to HIDDEN will have problem. since we look external symbol as csect

@DiggerLin, unless if this code is changing, the csect storage class is an issue. A small solution might be as simple as checking F->isDeclaration().

re: "the refactor":
A proliferation of code to set symbol properties for the csect of function descriptors seems like a sign of potential timing problems (in addition to the usual problems that come with duplicated logic).

[jasonliu]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1767

@jasonliu, do you mean that we should stop generating a label for the function descriptor in both the assembly writing and the direct object file writing paths?

We could choose not generating a label for function descriptor if we are sure that we are not going to reference the label anywhere and always use qualname.
But what I meant back there is that if we do this:

void foo() ;
void (*foo_ptr)() = &foo;

We do not need to generate the label for declaration foo(). (Yes, we will have the label if foo() is defined.)

Yes, we could use the F->isDeclaration() to determine if the csect is C_EXT or HIDDEN for now.

Thanks for the clarification of "the refactor". I don't think it's unavoidable, but definitely more than nice to have (for the timing problem).

[hubert.reinterpretcast]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1767

@jasonliu, thanks for the clarification.

We do not need to generate the label for declaration foo(). (Yes, we will have the label if foo() is defined.)

Agreed.

Yes, we could use the F->isDeclaration() to determine if the csect is C_EXT or HIDDEN for now.

Okay, let's get that into this patch. It might be ready if the change between revisions is relatively small.

Thanks for the clarification of "the refactor". I don't think it's unavoidable, but definitely more than nice to have (for the timing problem).

Okay.

[hubert.reinterpretcast]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1769

SC is calculated, but not necessarily used. It probably makes sense to move the conditional to where SC is set.

llvm/test/CodeGen/PowerPC/aix-reference-func-addr-const.ll

3

This still says pwr7.

[DiggerLin]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1769

changed , thanks

llvm/test/CodeGen/PowerPC/aix-reference-func-addr-const.ll

3

changed to pwr4 , thanks

[DiggerLin]

address comment

[hubert.reinterpretcast]

Just one minor issue with the patch itself, and otherwise, I don't see an issue with the patch in isolation.

I am, however, concerned about the testing. We are adding code to create a MCSectionXCOFF with various properties in this patch, and we don't seem to have the corresponding testing here. For example, how would the C_HIDDEN manifest itself if not corrected by code inspection? It is also difficult to be confident that a test would actually exercise this instance of the code that sets the csect symbol properties (as opposed to another instance). @DiggerLin, @jasonliu: It would be appreciated if the team can discuss the plan on handing this concern and the timeline on which we are expecting it to close. I would prefer not to have a "long tail" after this patch lands.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1767

This should be C_HIDEXT.

[DiggerLin]

change C_HIDDEN to C_HIDEXT

[daltenty]

Other than minor nit (and the follow on issue), this patch LGTM.

With regards to the testing:

In D71144#1791800, @hubert.reinterpretcast wrote:

I am, however, concerned about the testing. We are adding code to create a MCSectionXCOFF with various properties in this patch, and we don't seem to have the corresponding testing here. For example, how would the C_HIDDEN manifest itself if not corrected by code inspection? It is also difficult to be confident that a test would actually exercise this instance of the code that sets the csect symbol properties (as opposed to another instance).

This is one reason for the possible refactor mentioned before that moves creating these properties of an MCSectionXCOFF to a common place. It is difficult to see how we would have adequate test coverage otherwise, as you say there are too many code paths involved that may end up imbuing a particular MCSectionXCOFF with a different set of properties depending on how we got there. I think we should prioritize addressing this in a follow on patch.

llvm/test/CodeGen/PowerPC/aix-reference-func-addr-const.ll

15

nit: You can probably fold most of these check directives together with the 64-bit case by using a CHECK32 for the 32-bits parts and specifying multiple check prefixes. We are currently doing this in other tests that vary only slightly based on 32/64-bit.

[daltenty]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1771

This should be SectionKind::getData() if the function is defined

[hubert.reinterpretcast]

@jasonliu, @daltenty; as discussed off-list today, it would be much cleaner if we dropped producing a label definition for the function descriptor. In particular, I have validated my suspicions that referencing the DS csect in a file where the function is defined weak is problematic if the linkage is associated with the label definition and not the csect. Thank you both for tracking this on your TODO lists.

[hubert.reinterpretcast]

@daltenty, following up on needing to set linkage for csects in the assembly (in a more general case than just for function descriptors, as you pointed out): We are missing the linkage on the assembly path for weak references.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

 public:
   PPCAIXAsmPrinter(TargetMachine &TM, std::unique_ptr<MCStreamer> Streamer)
       : PPCAsmPrinter(TM, std::move(Streamer)) {}
 
   StringRef getPassName() const override { return "AIX PPC Assembly Printer"; }
 
   void SetupMachineFunction(MachineFunction &MF) override;
 
+  const MCExpr *lowerConstant(const Constant *CV) override;
+
   void EmitGlobalVariable(const GlobalVariable *GV) override;
 
   void EmitFunctionDescriptor() override;
 
   void EmitEndOfAsmFile(Module &) override;
 };
 
 } // end anonymous namespace
void PPCAIXAsmPrinter::ValidateGV(const GlobalVariable *GV) {
 
   if (GV->hasSection())
     report_fatal_error("Custom section for Data not yet supported.");
 
   if (GV->hasComdat())
     report_fatal_error("COMDAT not yet supported by AIX.");
 }
 
+const MCExpr *PPCAIXAsmPrinter::lowerConstant(const Constant *CV) {
+  if (const Function *F = dyn_cast<Function>(CV)) {
+    MCSymbolXCOFF *FSym = cast<MCSymbolXCOFF>(getSymbol(F));
+    if (!FSym->hasContainingCsect()) {
+      const XCOFF::StorageClass SC =
+          TargetLoweringObjectFileXCOFF::getStorageClassForGlobal(F);
+      MCSectionXCOFF *Csect = OutStreamer->getContext().getXCOFFSection(
+          FSym->getName(), XCOFF::XMC_DS,
+          F->isDeclaration() ? XCOFF::XTY_ER : XCOFF::XTY_SD, SC,
+          SectionKind::getMetadata());
+      FSym->setContainingCsect(Csect);
+    }
+    return MCSymbolRefExpr::create(
+        FSym->getContainingCsect()->getQualNameSymbol(), OutContext);
+  }
+  return PPCAsmPrinter::lowerConstant(CV);
+}
+
 void PPCAIXAsmPrinter::EmitGlobalVariable(const GlobalVariable *GV) {
   ValidateGV(GV);
 
   // External global variables are already handled.
   if (!GV->hasInitializer())
     return;
 
   // Create the symbol, set its storage class.
   MCSymbolXCOFF *GVSym = cast<MCSymbolXCOFF>(getSymbol(GV));
   GVSym->setStorageClass(

llvm/test/CodeGen/PowerPC/aix-reference-func-addr-const.ll

+; 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
+
+@foo_ptr = global void (...)* @foo
+declare void @foo(...) 
+
+@bar_ptr1 = global void (...)* bitcast (void ()* @bar to void (...)*)
+define void @bar() {
+entry:
+  ret void
+}
+
+
+;CHECK:          .csect .data[RW]
+;CHECK-NEXT:     .globl  foo_ptr
+;CHECK-NEXT:     .align  2
+;CHECK-NEXT:     foo_ptr:
+;CHECK-NEXT:     .long   foo[DS]
+;CHECK-NEXT:     .globl  bar_ptr1
+;CHECK-NEXT:     .align  2
+;CHECK-NEXT:     bar_ptr1:
+;CHECK-NEXT:     .long   bar[DS]
+
+;CHECK64:         .csect .data[RW]
+;CHECK64-NEXT:         .globl  foo_ptr
+;CHECK64-NEXT:         .align  3
+;CHECK64-NEXT:    foo_ptr:
+;CHECK64-NEXT:         .llong  foo[DS]
+;CHECK64-NEXT:         .globl  bar_ptr1
+;CHECK64-NEXT:         .align  3
+;CHECK64-NEXT:    bar_ptr1:
+;CHECK64-NEXT:         .llong  bar[DS]