This is an archive of the discontinued LLVM Phabricator instance.

[AIX] Emit TOC entries for ASM printing
ClosedPublic

Authored by daltenty on Nov 19 2019, 1:48 PM.

Details

Summary

Emit the correct .toc psuedo op when we change to the TOC and emit
TC entries. Make sure TOC psuedos get the right symbols via overriding
getMCSymbolForTOCPseudoMO on AIX. Add a test for TOC assembly writing
and update tests to include TOC entries.

Also make sure external globals have a csect set and handle external function descriptor (originally authored by Jason Liu) so we can emit TOC entries for them.

Diff Detail

Event Timeline

daltenty created this revision.Nov 19 2019, 1:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 19 2019, 1:48 PM
daltenty edited the summary of this revision. (Show Details)Nov 19 2019, 1:51 PM
jasonliu added inline comments.Nov 20 2019, 7:34 AM
llvm/include/llvm/MC/MCSymbolXCOFF.h
57

I think we have an exception here -> TOC[TC0]

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

TC entry itself is a csect. Do we need to create that csect(MCSectionXCOFF) and do a SwitchSection for every TC entry?
Otherwise, I will need some help on understanding how to implement object file generation for TC entry.

1866

If we are going to call EmitGlobalVariable for every GV, do we need to ValidateGV twice here? Or are there situations that we would call getMCSymbolForTOCPseudoMO for a particular GV that is not going to be checked in EmitGlobalVariable?

1896

Nit: Could we further reduce the nestness of this function by using early returns?

if (MO.getType() != MachineOperand::MO_GlobalAddress)
  return PPCAsmPrinter::getMCSymbolForTOCPseudoMO(MO);
llvm/lib/Target/PowerPC/PPCMCInstLower.cpp
67 ↗(On Diff #230142)

A thought here:
We are going to see this again in getMCSymbolForTOCPseudoMO anyway. Would it be better if we move the logic here to the getMCSymbolForTOCPseudoMO?
We would be able to combine the logic with undefined symbol there, and the only difference is that if we should put XCOFF::XMC_DS or XCOFF::XMC_UA into the MCSection.

daltenty marked an inline comment as done.Nov 21 2019, 8:12 AM
daltenty added inline comments.
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1849

I agree, this needs to be revised.

1866

The problem is that this will be called before EmitGlobalVariable, so we need to filter them here so we don't try to handle unsupported cases like thread locals, etc. But fatal erroring them here will not necessarily cover all the cases where we might emit them, since this will only cover those we have TOC references for, thus I believe we should cover both.

Just a reminder, since I landed the patch: https://reviews.llvm.org/D70243. You may need to update three testcases there.

DiggerLin added inline comments.Nov 22 2019, 8:05 AM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1880

instead of PPCAsmPrinter::getMCSymbolForTOCPseudoMO(MO)->getName(),
why not use getSymbol(MO.getGlobal())->getName() directory here ?

1881

we create a MCSectionXCOFF which has symbol type "XCOFF::XTY_ER", You have a test case which the external symbol, I just wonder it not hit the assert in the function MCSectionXCOFF construction?

assert((ST == XCOFF::XTY_SD || ST == XCOFF::XTY_CM) &&

"Invalid or unhandled type for csect.");
daltenty updated this revision to Diff 230690.Nov 22 2019, 10:55 AM
daltenty marked 2 inline comments as done.

Address review comments, including:

  • Changing the way we do spliting for unqualified names
  • Refactoring external FunDesc into getMCSymbolForTOCPseudoMO
  • Updating tests for presence of TC entries
daltenty added inline comments.Nov 22 2019, 10:55 AM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1881

I believe that is handled in the parent revision D70443

daltenty marked 3 inline comments as done.Nov 22 2019, 10:56 AM
jasonliu added inline comments.Nov 25 2019, 3:21 PM
llvm/include/llvm/MC/MCAsmInfo.h
337

nit: Do we need to capitalize Commons and Storage Mapping Class? I find them all in small letters in AIX assembler guide.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp
33

If we intend to keep this interface change:
emitTCEntry(const MCSymbol &, const MCSymbol &)

We could remove this include directive.

111–112

I'm on the fence of this interface change.
I could see this interface change makes emitTCEntry a bit cleaner, and it models the TC entry a bit better: a symbol represent the real TC, and a symbol represent the item that TC is trying to store.
But on the other hand, I don't think we are ever getting ".tc A[TC], B", and we always have ".tc A[TC], A" (at least for now). Using the new interface would kinda make that implicit connection lost.

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

Could be a static member function?

1909

Could we use this structure for the function? I think it's easier to read and understand.

if (MO.getType() != MachineOperand::MO_GlobalAddress)
  return PPCAsmPrinter::getMCSymbolForTOCPseudoMO(MO);

const GlobalVariable *GV = MO.getGlobal();
ValidateGV(GV)
if (GV->isDeclaration()) {
  if (!Xsym->hasContainingCsect()) {
    // Set up csect for external symbol, use isa<Function> to figure out what SMC we should use for the csect. 
  }
  return Csect->getQualNameSymbol();
}

SectionKind GVKind = getObjFileLowering().getKindForGlobal(GV, TM);
if (GVKind.isCommon() || GVKind.isBSSLocal()) {
  return Csect->getQualNameSymbol();
}

return getSymbol(GV);

Some remark on some implication on this change:

!(GO = dyn_cast<const GlobalObject>(MO.getGlobal())

I'm not sure why we need this dyn_cast on the first if, so I removed it.

const GlobalVariable *GV = dyn_cast<const GlobalVariable>(GO);
  if (GV) {
    ValidateGV(GV);

This is a upcast, it's definitely going to succeed. So we don't need dyn_cast, and we will get a GV for sure.

  1. On your current function, it will return a qual name for a defined function. And this structure will not return a qual name for the defined function. I think the qual name for the defined function is unnecessary, and use label to refer to that function will succeed. And it's easier on the mental model for when we should use a qual name for TC entry: when the symbol is undefined, or when it's common.
daltenty marked an inline comment as done.Nov 26 2019, 8:29 AM
daltenty added inline comments.
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1909

I have a few issues with the specifics of this, as outlined bellow, but I think I can generally adopt this direction. I agree some of the GlobalVariable && Function case can just be folded so I'll try to follow that.

First on the remarks:

  1. MO.getGlobal() returns a GlobalValue, but we only want to handle GlobalObject cases (i.e. GlobalVariable & Function), so some kind of cast is required
  1. This is not an upcast, GO is a GlobalObject.
  1. Hmm, I actually found it easier to reason about referring to the CSects directly when appropriate, which is why I included that case.

Essentially, it boils down to "If you're talking about something that is a whole CSect in it's own right (i.e. Function Descriptors), refer to by it's QualNameSymbol". Why refer to all these extra middle-men label symbols when they aren't needed?

That said, if we prefer to keep this minimal and use the existing label, I can add in the checks to pass it by.

daltenty marked an inline comment as not done.Nov 26 2019, 9:22 AM
jasonliu added inline comments.Nov 26 2019, 10:36 AM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1909
  1. MO.getGlobal() returns a GlobalValue, but we only want to handle GlobalObject cases (i.e. GlobalVariable & Function), so some kind of cast is required

I see. I think the behavior is still correct because it would fall back to the last "return getSymbol(GV);", which is what PPCAsmPrinter::getMCSymbolForTOCPseudoMO(MO) do anyway. But I agree we could just do the early return.

  1. This is not an upcast, GO is a GlobalObject.

You are right. I think I mis-read the GV as GlobalValue. (I need to be careful when I see "GV" because it could mean GlobalValue or GlobalVariable.)

  1. Essentially, it boils down to "If you're talking about something that is a whole CSect in it's own right (i.e. Function Descriptors), refer to by it's QualNameSymbol".

Okk, I'm Okay with the mental model as "If we could refer to it as a csect, then let's refer to it as a csect.".

daltenty marked 3 inline comments as done.Nov 26 2019, 11:44 AM
daltenty added inline comments.
llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp
111–112

If that relationship is something we want to preserve, we could add an assert of the form:

assert(Entry.getName().equals(MAI->getSymbolsHaveSMC() ? Target.getUnqualifiedName()  : Target.getName()) && "TOC entries should normally have the same name as their targets")
jasonliu added inline comments.Nov 26 2019, 12:05 PM
llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp
111–112

Then do we really need two MCSymbol as parameters? I feel like it just complicates the relationship. And one MCSymbol is enough, even on AIX.

daltenty marked 2 inline comments as done.Nov 26 2019, 2:35 PM
daltenty added inline comments.
llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp
111–112

I think it make sense that we need both, it models the actual TC entry better as you said. And using one also presents difficulties in the AIX case.

We can't the qualname symbol, because this will not work for the object writing case as we need the value. But if we proceed with just the target symbol we end up rebuilding the same string we already had in the qualname symbol to begin with, since it's the same as the TC CSect (as we would expect).

If we want to maintain the old interface, for the PPC LE target where they are the same symbol, we just create a wrapper where duplicate the argument?

jasonliu added inline comments.Nov 26 2019, 8:21 PM
llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp
111–112

Maybe calling getUnqualifiedName() again is not that bad. But I'm Okay with what we have right now in the patch. The assert and wrapper does not really make things better.

DiggerLin added inline comments.Nov 27 2019, 7:20 AM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1892

it need to GV->isDeclaration(), otherwise a test case which as
int g=0;
will has storage mapping class XMC_UA.

DiggerLin added inline comments.Nov 27 2019, 8:38 AM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
45

file llvm/IR/GlobalVariable.h already include the GlobalObject.h.

the #include "llvm/IR/GlobalObject.h" do not need.

1463

I agree with jason, since we always have ".tc A[TC], A", we do not need to change the interface of function emitTCEntry()

daltenty marked 9 inline comments as done.Nov 27 2019, 8:54 AM
daltenty added inline comments.
llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp
111–112

Taking another look, I agree. I will update it to just call getUnqualifiedName() again.

daltenty updated this revision to Diff 231275.Nov 27 2019, 8:55 AM
daltenty marked an inline comment as done.

Address review comments round 2 including:

  • Minor edits
  • Don't change the emitTCEntry interface
  • Refactor getMCSymbolForTOCPseudoMO
daltenty updated this revision to Diff 231278.Nov 27 2019, 9:04 AM
  • Remove unneeded header
daltenty marked 2 inline comments as done.Nov 27 2019, 9:05 AM
jasonliu added inline comments.Nov 27 2019, 12:44 PM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1766

"Handle ... later" sounds confusing for an early return.
It's more like, we don't need to do anything for external global variables.
And this early return could be just after "ValidateGV(GV);", I think we already set storage class for the symbol in getMCSymbolForTOCPseudoMO. Is that right?

1849

nit: Csect -> csect

1863

Assign a nullptr to it?

1873

We used *GV again below, we could avoid dyn_cast the second time below if we make it a local variable.
And could *GV be const?

1879

nit: the global value -> the GlobalObject

1880

nit: "then MCSymbol XSym represents an external" -> "then XSym is an external referenced symbol."

1882

nit: it's -> its

1897

inialized globals-> initialized global variables

1907

We could add a comment here about other global variables, something like:
Other global variables are stored as labels inside of data/text section, we could not refer to them by qualname.

1911

nit: refer to the descriptor -> refer to the function descriptor csect.

daltenty marked 10 inline comments as done.Nov 27 2019, 1:11 PM
daltenty added inline comments.
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1766

Yes, that's correct. We end up covering this coincidentally.

daltenty updated this revision to Diff 231313.Nov 27 2019, 1:11 PM
  • Move early return.
  • Address other comments.
jasonliu accepted this revision.Nov 27 2019, 1:36 PM

Minor nit that could be address while landing.
Otherwise LGTM.

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

nit: Add some spaces here.

1873

Could GV be const here?

1882

... for it so that we...

This revision is now accepted and ready to land.Nov 27 2019, 1:36 PM
daltenty marked 3 inline comments as done.Nov 27 2019, 2:17 PM
This revision was automatically updated to reflect the committed changes.