This is an archive of the discontinued LLVM Phabricator instance.

[AIX] discard the label in the csect of function description and use qualname for linkage
ClosedPublic

Authored by DiggerLin on Mar 13 2020, 2:15 PM.

Details

Summary

In current llvm,

cat > test.c
void foo() {};

llc will generate assembly code as (assembly patch)
        .globl  foo
        .globl  .foo
        .csect foo[DS]
foo:
        .long   .foo
        .long   TOC[TC0]
        .long   0
and symbol table as (xcoff object file)
[4]     m   0x00000004     .data     1  unamex                    foo
[5]     a4  0x0000000c       0    0     SD       DS    0    0
[6]     m   0x00000004     .data     1  extern                    foo
[7]     a4  0x00000004       0    0     LD       DS    0    0


After first patch, the assembly will be as
```
     .globl  foo[DS]                 # -- Begin function foo
     .globl  .foo
     .align  2
     .csect foo[DS]
     .long   .foo
     .long   TOC[TC0]
     .long   0
    and symbol table will as 
   [6]     m   0x00000004     .data     1  extern                    foo
   [7]     a4  0x00000004       0    0     DS      DS    0    0

Change the code  for the assembly path and xcoff objectfile patch for llc.

Diff Detail

Event Timeline

DiggerLin created this revision.Mar 13 2020, 2:15 PM
DiggerLin retitled this revision from [AIX] discard the label of the function description and use qualname for linkage to [AIX] discard the label in the csect of function description and use qualname for linkage .Mar 13 2020, 2:20 PM
DiggerLin updated this revision to Diff 250575.Mar 16 2020, 8:51 AM

change .globl symbolName
to .globl qualSymbolName
in the test cases.

DiggerLin edited the summary of this revision. (Show Details)Mar 16 2020, 8:52 AM
DiggerLin edited the summary of this revision. (Show Details)
jasonliu added inline comments.Mar 20 2020, 8:40 AM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
1977

MCSymbolXCOFF already have a getStorageClass() function, I don't think we need to pass in GlobalObject to get it. If we need to, then we get the wrong FuncSym.

1979

FuncSym->getName() works means something is wrong here.
I assume FuncSym here is "foo", however FuncSym is supposed to be a "foo[DS]" name.
It's werid for us to have a symbol named "foo" here since we don't actually need that anywhere now after this patch.

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

This is similar issues I saw above, I think we have the wrong MCSymbol here. If we have the correct one, we could just call emitLinkage without this if statement.

DiggerLin marked 6 inline comments as done.Mar 22 2020, 6:08 PM
DiggerLin added inline comments.
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
1977

we do not set StorageClass() for the FuncSym before we call the getSectionForFunctionDescriptor. We can not get getStorageClass() before set it.

1979

The FuncSym is symbol in the MCContext. not in the MCAssembler. it is foo[DS] in the MCAssembler (we put the foo[DS] into MCAssembler when we emiltLinkage ). we only care about MCAssembler in the XCOFFObjectWriter::executePostLayoutBinding()

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

when we do to the emitLinkage in the doFinalization()
there is function .llvm.StackProtector which do not have hasContainingCsect()

DiggerLin updated this revision to Diff 251930.Mar 22 2020, 6:11 PM
DiggerLin marked 3 inline comments as done.
jasonliu added inline comments.Mar 23 2020, 9:36 AM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
1977

I think we should modify getSectionForFunctionDescriptor to only take const Function * as argument instead. You could get everything you need from that alone.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1542–1543

CurrentFnDescSym is foo here, which is wrong. It should be foo[DS].
You can achieve that by moving this after getting FnDescSec:
CurrentFnDescSym = FnDescSec->getQualNameSymbol();

1547

What I meant is, GVSym we received from function argument, should be foo[DS], or foo depending which one we need. We shouldn't try to get the correct one here. It's the caller's responsibility to pass in the correct MCSymbol. And in this patch, it's not even necessary to override emitLinkage for AIX.

DiggerLin updated this revision to Diff 252354.Mar 24 2020, 9:47 AM
DiggerLin marked 2 inline comments as done.

address comment

DiggerLin marked 2 inline comments as done.Mar 24 2020, 9:48 AM
jasonliu added inline comments.Mar 24 2020, 10:48 AM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
1979

Looking at what other XCOFF function is doing in this file, I think we should be consistent with them for easier maintenance which means we should call

getNameWithPrefix(Name, GO, TM);

instead.

1981

NameStr is good. I don't think you need to call .str().

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

Why do we use dyn_cast? Is there a chance GO is not a Function when we gets here?

DiggerLin marked 6 inline comments as done.Mar 24 2020, 3:32 PM
DiggerLin added inline comments.
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
1979

TM is not a member of TargetLoweringObjectFile, do you want pass TM as parameter of getSectionForFunctionDescriptor() ? I do not think we need a TM as parameter of getSectionForFunctionDescriptor.

1981

thanks, there is function operator StringRef() const { return str(); }

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

getSectionForFunctionDescriptor(Function*) , but GO is GlobalObject.

jasonliu added inline comments.Mar 24 2020, 7:39 PM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
1979

I think it makes sense to pass it in. I understand right now calling to getNameWithPrefix(Name, GO, TM) is the same as what you have which is getMangler()->getNameWithPrefix(OutName, GV, /*CannotUsePrivateLabel=*/false);. But if later we decide to override getNameWithPrefix(Name, GO, TM) for XCOFF, we will forget there is this one place called directly to getMangler()->getNameWithPrefix .

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

Let me rephrase my question, could you use cast instead of dyn_cast?

DiggerLin updated this revision to Diff 252568.Mar 25 2020, 7:09 AM
DiggerLin marked 5 inline comments as done.

address comment

Some last cleanups needed...

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1542–1543

We should modify the comment a bit to something like:
// Setup CurrentFnDescSym and its containing csect.

1548–1549

This line could be removed, as the symbol returned from getQualNameSymbol() should already have its csect set.

1567–1568

I think we could remove this FSym now, as it's not needed and it's better if we actually remove its existant from MCContext.
This if block could rewrite to:

if (const Function *F = dyn_cast<Function>(CV)) {
  const MCSectionXCOFF *Csect = cast<MCSectionXCOFF>(
          F->isDeclaration()
              ? getObjFileLowering().getSectionForExternalReference(F, TM)
              : getObjFileLowering().getSectionForFunctionDescriptor(F, TM));
   return MCSymbolRefExpr::create(Csect->getQualNameSymbol(), OutContext);
 }
DiggerLin updated this revision to Diff 252595.Mar 25 2020, 9:12 AM
DiggerLin marked 3 inline comments as done.
DiggerLin added inline comments.Mar 25 2020, 9:20 AM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1548–1549

thanks

1737

OK

jasonliu accepted this revision.Mar 25 2020, 9:33 AM

There is a nit, otherwise LGTM.

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

clang format this block, I don't think the spaces are aligned correctly.

This revision is now accepted and ready to land.Mar 25 2020, 9:33 AM
This revision was automatically updated to reflect the committed changes.
jasonliu added inline comments.Mar 26 2020, 2:10 PM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1568

This comment is not addressed when you land the commit.

jasonliu added inline comments.Mar 27 2020, 9:30 AM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1568

Could we land a commit to address this before we all forget? Please comment if you landed this change. Thanks.

DiggerLin marked 3 inline comments as done.Mar 27 2020, 1:22 PM
DiggerLin edited the summary of this revision. (Show Details)Apr 15 2020, 1:44 PM
DiggerLin edited the summary of this revision. (Show Details)