Page MenuHomePhabricator

[AIX][XCOFF] change the operand of branch instruction from symbol name to qualified symbol name for function declarations
ClosedPublic

Authored by DiggerLin on Jul 28 2020, 9:35 AM.

Details

Summary
  1. in the patch , remove setting storageclass in function .getXCOFFSection and construct function of class MCSectionXCOFF

there are

XCOFF::StorageMappingClass MappingClass;
XCOFF::SymbolType Type;
XCOFF::StorageClass StorageClass;
in the MCSectionXCOFF class,
these attribute only used in the XCOFFObjectWriter, (asm path do not need the StorageClass)

we need get the value of StorageClass, Type,MappingClass before we invoke the getXCOFFSection every time.

actually , we can get the StorageClass of the MCSectionXCOFF from it's delegated symbol.

  1. we also change the oprand of branch instruction from symbol name to qualify symbol name.

for example change
bl .foo
extern .foo
to
bl .foo[PR]
extern .foo[PR]

  1. and if there is reference indirect call a function bar.

we also add

extern .bar[PR]

Diff Detail

Unit TestsFailed

TimeTest
60 mslinux > Polly.Isl/Ast::alias_checks_with_empty_context.ll
Script: -- : 'RUN: at line 1'; opt -polly-process-unprofitable -polly-remarks-minimal -polly-use-llvm-names -polly-import-jscop-dir=/mnt/disks/ssd0/agent/llvm-project/polly/test/Isl/Ast -polly-codegen-verify -polly-ast -analyze < /mnt/disks/ssd0/agent/llvm-project/polly/test/Isl/Ast/alias_checks_with_empty_context.ll | FileCheck /mnt/disks/ssd0/agent/llvm-project/polly/test/Isl/Ast/alias_checks_with_empty_context.ll

Event Timeline

DiggerLin created this revision.Jul 28 2020, 9:35 AM
DiggerLin requested review of this revision.Jul 28 2020, 9:35 AM
DiggerLin retitled this revision from [NFC][AIX][XCOFF] remove setting storageclass in function .getXCOFFSection and construct function of class MCSectionXCOFF to [AIX][XCOFF] change oprand of branch instruction from symbol name to qualify symbol name..Aug 5 2020, 8:53 AM
DiggerLin edited the summary of this revision. (Show Details)
DiggerLin updated this revision to Diff 283319.Aug 5 2020, 11:54 AM
DiggerLin added a reviewer: sfertile.
jasonliu added inline comments.Aug 5 2020, 1:32 PM
llvm/include/llvm/MC/MCSectionXCOFF.h
66

How necessary is this convenient function? It might be better to just let people do the call to QualName themselves to avoid confusion, as storage class is not a property of MCSection any more.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1535

After this change, do we still have any call to hasRepresentedCsectSet? If not, could we remove that function all together?

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
2167

comment needs to be update here.

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

There seems to be a switch of execution order here, any reason for that?

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

I don't think it's necessary to change the name, as the sister function in TLOF have the same name but still return qualname when needed.

5176

Dont use twine in local. You could do

Context.getXCOFFSection(("." + Twine(SymName)).str(), ....
llvm/test/CodeGen/PowerPC/aix-xcoff-toc.ll
138

nit: align the spaces.

As you mentioned in the summary, this patch seems be addressing two orthogonal things:

  1. remove SC from MCSectionXCOFF and let it only be a property of MCSymbolXCOFF
  2. when branching to an undefined function, branch to qualname instead.

If I am right, I would suggest we split them into two patches.

And for the second patch when branching to an undefined function, branch to qualname instead, I think we need to adjust the patch title to a more accurate one[maybe [AIX][XCOFF] change the operand of branch instruction from symbol name to qualified symbol name for function declarations]since the scope is actually limited to function declarations only.

llvm/include/llvm/MC/MCSectionXCOFF.h
50

I am wondering why do we want to set a default SC here? It seems redundant.

llvm/include/llvm/MC/MCSymbolXCOFF.h
38

It looks like this assertion is still useful to prevent someone from setting SC twice or setting other SC for a same symbol accidentally, if we don't need to set a default SC as I mentioned above.

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

Please update the comment accordingly.

jasonliu added inline comments.Aug 6 2020, 9:06 AM
llvm/include/llvm/MC/MCSectionXCOFF.h
50

We transfer SC property from MCSectionXCOFF to MCSymbolXCOFF. MCSectionXCOFF represents a csect. A csect by default have XCOFF::C_HIDEXT Storage class. Without this, we would not be able to get storage class from a MCSectionXCOFF when that csect do not need to emitLinkage.

llvm/include/llvm/MC/MCSymbolXCOFF.h
38

If I understand correctly, we would actually set MCSectionXCOFF/csect's SC twice in some scenarios. First time being the default HIDE_EXT. Second time, when someone called emitLinkage on a csect's qualname symbol (and we could set it to C_EXT at that time.

Xiangling_L added inline comments.Aug 6 2020, 10:53 AM
llvm/include/llvm/MC/MCSectionXCOFF.h
50

Did you mean emitSymbolAttribute? I guess this matters more on object file generation path? Since on assembly path, `The .lglobl pseudo-op does not have to apply to any csect name. The assembler automatically generates the symbol table entry for any csect name with a class of C_HIDEXT unless there is an
explicit .globl pseudo-op applied to the csect name.`

jasonliu added inline comments.Aug 6 2020, 11:02 AM
llvm/include/llvm/MC/MCSectionXCOFF.h
50

emitLinkage would call emitSymbolAttribute on the object generation path. And yes, this matters more on object generation path.

DiggerLin marked 4 inline comments as done.Aug 6 2020, 1:26 PM
DiggerLin added inline comments.
llvm/include/llvm/MC/MCSectionXCOFF.h
66

what about change the function name to getQualNameSymbolStorageClass() ? I think we need a wrapper for it.

llvm/include/llvm/MC/MCSymbolXCOFF.h
38

yes, we need to set twice as jason explain

DiggerLin retitled this revision from [AIX][XCOFF] change oprand of branch instruction from symbol name to qualify symbol name. to [AIX][XCOFF] [AIX][XCOFF] change the operand of branch instruction from symbol name to qualified symbol name for function declarations.Aug 6 2020, 2:50 PM
DiggerLin marked 12 inline comments as done.Aug 7 2020, 9:26 AM
DiggerLin added inline comments.
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1906

for the object path, the function executePostLayoutBinding() and writeObject() was called in doFinalization() indirectly, we need to run

for (MCSymbol *Sym : ExtSymSDNodeSymbols)
   OutStreamer->emitSymbolAttribute(Sym, MCSA_Extern);

to set the symbol atttribute before PPCAsmPrinter::doFinalization.

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

MCSymbol *TargetLoweringObjectFileXCOFF::getFunctionEntryPointSymbol() return .foo not .foo[PR] .
in order to not to confuse ,I use the name getFunctionEntryPointQualNameSymbol() to get .foo[PR]

5176

agree. changed

DiggerLin marked 2 inline comments as done.EditedAug 7 2020, 9:46 AM

As you mentioned in the summary, this patch seems be addressing two orthogonal things:

  1. remove SC from MCSectionXCOFF and let it only be a property of MCSymbolXCOFF
  2. when branching to an undefined function, branch to qualname instead.

If I am right, I would suggest we split them into two patches.

And for the second patch when branching to an undefined function, branch to qualname instead, I think we need to adjust the patch title to a more accurate one[maybe [AIX][XCOFF] change the operand of branch instruction from symbol name to qualified symbol name for function declarations]since the scope is actually limited to function declarations only.

As you mentioned in the summary, this patch seems be addressing two orthogonal things:

  1. remove SC from MCSectionXCOFF and let it only be a property of MCSymbolXCOFF
  2. when branching to an undefined function, branch to qualname instead.

If I am right, I would suggest we split them into two patches.

And for the second patch when branching to an undefined function, branch to qualname instead, I think we need to adjust the patch title to a more accurate one[maybe [AIX][XCOFF] change the operand of branch instruction from symbol name to qualified symbol name for function declarations]since the scope is actually limited to function declarations only.

As you mentioned in the summary, this patch seems be addressing two orthogonal things:

  1. remove SC from MCSectionXCOFF and let it only be a property of MCSymbolXCOFF
  2. when branching to an undefined function, branch to qualname instead.

If I am right, I would suggest we split them into two patches.

And for the second patch when branching to an undefined function, branch to qualname instead, I think we need to adjust the patch title to a more accurate one[maybe [AIX][XCOFF] change the operand of branch instruction from symbol name to qualified symbol name for function declarations]since the scope is actually limited to function declarations only.

you can take a look on the first diff of the patch. I tried to do "remove SC from MCSectionXCOFF and let it only be a property of MCSymbolXCOFF" only in the first diff.
if only remove SC from MCSectionXCOFF, the emitLinkage set the storageClass of the symbol which is not qualname represent symbol of the MCSectionXCOFF. I need to add
some middle source code (in llvm/lib/MC/MCXCOFFStreamer.cpp of first diff) to set the storageClasss of the qualname represent symbol of the MCSectionXCOFF. and the code will be removed later when we implement the second patch. It will waste our time to review.

DiggerLin updated this revision to Diff 283946.Aug 7 2020, 10:30 AM

address comment

DiggerLin retitled this revision from [AIX][XCOFF] [AIX][XCOFF] change the operand of branch instruction from symbol name to qualified symbol name for function declarations to [AIX][XCOFF] change the operand of branch instruction from symbol name to qualified symbol name for function declarations.Aug 7 2020, 12:33 PM
jasonliu added inline comments.Aug 7 2020, 12:37 PM
llvm/include/llvm/MC/MCSectionXCOFF.h
66

Please try to address all the clang-format/clang-tidy message.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
5171–5172

Suggestion for the comment:
...... C-linkage name. A Qualname is returned here because an external function entry point is a csect with XTY_ER property.

5172

By looking at what this function does. It seems the name should be getExternalFunctionEntryPointSymbol, as it's always returning a function entry point to an external function. That's an important information that we should include in the name.

DiggerLin updated this revision to Diff 284071.Aug 7 2020, 3:04 PM
DiggerLin marked an inline comment as done.
jasonliu added inline comments.Aug 10 2020, 6:43 AM
llvm/include/llvm/MC/MCSymbolXCOFF.h
51

nit: For the implementation of getRepresentedCsect and setRepresentedCsect, please remove

|| RepresentedCsect->getCSectType() == XCOFF::XTY_ER

and

|| C->getCSectType() == XCOFF::XTY_ER

from their asserts, as right now every RepresentedCsect symbol must be a qualname.

address comment

jasonliu accepted this revision.Aug 10 2020, 10:43 AM

Thanks. LGTM.

This revision is now accepted and ready to land.Aug 10 2020, 10:43 AM
This revision was landed with ongoing or failed builds.Aug 11 2020, 12:26 PM
This revision was automatically updated to reflect the committed changes.