This is an archive of the discontinued LLVM Phabricator instance.

[AIX] Use csect reference for function address constants
ClosedPublic

Authored by DiggerLin on Dec 6 2019, 12:36 PM.

Details

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.

Diff Detail

Event Timeline

DiggerLin created this revision.Dec 6 2019, 12:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 6 2019, 12:36 PM
Xiangling_L added inline comments.Dec 9 2019, 5:43 PM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1770

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.

1775

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.

DiggerLin edited the summary of this revision. (Show Details)Dec 10 2019, 8:03 AM
daltenty added inline comments.Dec 10 2019, 10:26 AM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1770

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.

1780

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?

DiggerLin marked 4 inline comments as done.

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.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1769

Shouldn't this just be:

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

?

1786

I suggest to name the direct base class:

return PPCAsmPrinter ::lowerConstant(CV);
1787

Empty line between function bodies please.

llvm/test/CodeGen/PowerPC/aix-xcoff-FuntionDS.ll
1 ↗(On Diff #232631)

Why pwr7?

Xiangling_L added inline comments.
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?

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 edited the summary of this revision. (Show Details)Dec 16 2019, 12:29 PM
DiggerLin retitled this revision from Not emit correct Assembly for Global Function pointer initiated with function. to [AIX] Use csect reference for function address constants.
DiggerLin marked 10 inline comments as done.Dec 16 2019, 12:58 PM
DiggerLin added inline comments.
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1769

good idea, change as suggestion.

1770

agree thanks

1775

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

1786

changed as suggestion

llvm/test/CodeGen/PowerPC/aix-xcoff-FuntionDS.ll
1 ↗(On Diff #233142)

changed file names

DiggerLin marked 3 inline comments as done.
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1775

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 marked an inline comment as done.Dec 17 2019, 10:24 AM
DiggerLin added inline comments.
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1775

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 marked an inline comment as done.Dec 17 2019, 12:20 PM
DiggerLin added inline comments.
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1775

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 added inline comments.Dec 17 2019, 12:34 PM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1775

@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 added inline comments.Dec 17 2019, 1:55 PM
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.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1775

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 added inline comments.Dec 18 2019, 8:37 AM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1775

@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).

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1775

@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.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1777

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 marked 4 inline comments as done.Dec 19 2019, 12:18 PM
DiggerLin added inline comments.
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1777

changed , thanks

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

changed to pwr4 , thanks

DiggerLin marked 2 inline comments as done.

address comment

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
1775

This should be C_HIDEXT.

DiggerLin marked an inline comment as done.Dec 23 2019, 8:43 AM
DiggerLin updated this revision to Diff 235149.Dec 23 2019, 8:52 AM

change C_HIDDEN to C_HIDEXT

daltenty added a comment.EditedJan 2 2020, 9:07 AM

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

With regards to the testing:

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 accepted this revision.Jan 3 2020, 8:59 AM
This revision is now accepted and ready to land.Jan 3 2020, 8:59 AM
daltenty added inline comments.Jan 3 2020, 12:34 PM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1779

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

This revision was automatically updated to reflect the committed changes.

@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.

@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.