Page MenuHomePhabricator

[NFC][XCOFF][AIX] Refactor get/setContainingCsect
ClosedPublic

Authored by jasonliu on Mar 30 2020, 12:16 PM.

Details

Summary

For current architect, we always require setContainingCsect to be called on every MCSymbol got used in XCOFF context.
This is very hard to achieve because symbols gets created everywhere and other MCSymbol types(ELF, COFF) do not have similar rules.
It's very easy to miss setting the containing csect, and we would need to add a lot of XCOFF specialized code around some common code area.

This patch intendeds to do

  1. Rely on getFragment().getParent() to get csect from labels.
  2. Only use get/setRepresentedCsect (was get/setContainingCsect) if symbol itself represents a csect.

Diff Detail

Event Timeline

jasonliu created this revision.Mar 30 2020, 12:16 PM
jasonliu edited the summary of this revision. (Show Details)Mar 30 2020, 12:43 PM
DiggerLin added inline comments.Mar 31 2020, 10:00 AM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1609

I think we still need GVSym->setContainingCsect(Csect) for extern variable . when we emitlLnkage for the extern Variable, the GVSym is not a qualName symbol, So we get the SMC from GVSym's containningCsect.

for example:
extern int b_e;
we need to emit linkage as
.extern b_e[UA]

DiggerLin added inline comments.Mar 31 2020, 10:01 AM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1609

I think we can change to
if ( GV->isDeclaration()) {

GVSym->setContainingCsect(Csect);

}

jasonliu marked an inline comment as done.Mar 31 2020, 10:46 AM
jasonliu added inline comments.
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1609

Symbol b_e[UA] will have getCsectMCSectionXCOFF returning the csect you want. It's a qualname, so it will have the old "containingCsect", or the new "CsectMCSectionXCOFF".

jasonliu marked an inline comment as done.Mar 31 2020, 10:48 AM
jasonliu added inline comments.
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1609

The GVSym you there better be b_e[UA].

This revision is now accepted and ready to land.Apr 1 2020, 1:36 PM
llvm/include/llvm/MC/MCSymbolXCOFF.h
58

Suggestion: RepresentedCsect.

daltenty added inline comments.Apr 1 2020, 5:01 PM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
5145

It feels like this breaks the constraint you had listed under 2, since the symbol S is not actually the symbol representing the csect (i.e. the qualname symbol). Would we be able to wrap this whole business in a call to an new TLOF function like TargetLoweringObjectFileXCOFF::getSectionForUndefinedFunction(StringRef) and just use the qualname symbol we get back instead (so we don't need to call setCsectMCSectionXCOFF() seperately)?

If we can do this we could restrict setCsectMCSectionXCOFF() so it can only be called from MCSectionXCOFF's constructor, enforcing the constraint that this only valid for symbols representing csects (and hasCsectMCSectionXCOFF() could become isCsect() ).

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1849

Can we expect that hasCsectMCSectionXCOFF is true for an undefined XCOFF symbol? Do we want to assert that?

llvm/lib/CodeGen/MachineModuleInfo.cpp
107–108

This is unused except by the assert.

llvm/lib/MC/MCSymbolXCOFF.cpp
17

Just noting that the setter already checks this, which explains why the assert does not need to be written in terms of what the callee is trying to do--it can be written in terms of the internal state of this object.

19

Suggestion:
Symbol does not represent a csect; MCSectionXCOFF that represents the symbol should not be (but is) set.

24

Should also assert that C is not null.

29

Suggestion:
Symbol does not represent a csect; can only set a MCSectionXCOFF representation for a csect.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
5145

Since the symbol is undefined. It must represent the csect. I also would prefer if we can have a stronger correspondence between being a QualName symbol and being the representation of a csect, but I am not sure how much that entails.

jasonliu marked an inline comment as done.Apr 1 2020, 6:03 PM
jasonliu added inline comments.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
5145

I think I'm going to use the same reason why this haven't been done in D72347: we do not have the access to TLOF here.
Aside from that, I had the same debate that you have. The conclusion I came up with is that all the undefined symbol are essentially csects (even though that undefined symbol is actually a label in the other compilation unit). So in here, although we do not have a qualname, this symbol still represents a csect.
I agree with you that it might be better if we have a qualname .bl .foo[PR] here when foo is an undefined function. So that we know if we have a qualname, it must be csect. But right now, I don't see an immediate need for that and I kinda want to leave things as it is until there are concrete reasons to change it.

jasonliu marked an inline comment as done.Apr 2 2020, 8:24 AM
jasonliu added inline comments.
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1849

The original purpose of hasCsectMCSectionXCOFF should really be hasCsectMCSectionXCOFFSet. So it won't always be true. I will change the name to hasCsectMCSectionXCOFFSet to avoid confusion. I'm not sure if we really need hasCsectMCSectionXCOFF though, maybe not for this patch until we find the reason that we need to.

jasonliu updated this revision to Diff 254553.Apr 2 2020, 9:52 AM
jasonliu marked 2 inline comments as done.

Rename and address comment.

jasonliu edited the summary of this revision. (Show Details)Apr 2 2020, 9:52 AM
jasonliu marked 4 inline comments as done.
daltenty accepted this revision.Apr 2 2020, 9:19 PM

LGTM

This revision was automatically updated to reflect the committed changes.