Page MenuHomePhabricator

[AIX] Implement function descriptor on SDAG
ClosedPublic

Authored by Xiangling_L on May 28 2019, 10:13 AM.

Details

Summary

(1)Function descriptor on AIX
On AIX, a called routine may have 2 distinct symbols associated with it:

  • A function descriptor (Name)
  • A function entry point (.Name)

The descriptor structure on AIX is the same as those in the ELF V1 ABI:

  • The address of the entry point of the function.
  • The TOC base address for the function.
  • The environment pointer.

The descriptor symbol uses the same name as the source level function in C.
The function entry point is analogous to the symbol we would generate for a function in a
non-descriptor-based ABI, except that it is renamed by prepending a ".".

Which symbol gets referenced depends on the context:

  • Taking the address of the function references the descriptor symbol.
  • Calling the function references the entry point symbol.

(2)Speaking of implementation on AIX, for direct function call target, we create proper MCSymbol SDNode(e.g . ".foo") while constructing SDAG to replace original TargetGlobalAddress SDNode. Then down the path, we can take advantage of this MCSymbol.

(3)And since -stop-after llc option is used to test MIR results in the LIT testcase, to be able to get a MCSymbolXCOFF in SDAG layer, an option "-init-MC" is added to use together with -stop-after/-stop-before option to force llc doing the target object file lowering.

Diff Detail

Repository
rL LLVM

Event Timeline

Xiangling_L created this revision.May 28 2019, 10:13 AM

The commit message should explain something about function descriptor symbols on AIX and how they differ from the symbol that is created for the function entry point.

llvm/lib/CodeGen/LLVMTargetMachine.cpp
207 ↗(On Diff #201684)

Use clang-format (pay attention to the spaces):

const_cast<TargetLoweringObjectFile &>(*this->getObjFileLowering())
    .Initialize(Ctx, *this);
llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
1878 ↗(On Diff #201684)

Is this statement of fundamental (permanent) fact? Please make the comment clear on that point. Clarify the relation between the first and the second sentences. For example, does the second sentence indicate something that is only done if we return false for AIX?

1879 ↗(On Diff #201684)

s/functions/function's/;

1880 ↗(On Diff #201684)

Split out the last part into a separate sentence:
The latter is the symbol whose XCOFF symbol name is the C-linkage name of the source level function.

llvm/lib/CodeGen/TargetPassConfig.cpp
203 ↗(On Diff #201684)

Use clang-format:

static cl::opt<bool>
    InitMC("init-MC", cl::Hidden,
           cl::desc("Init target object file lowering when in use together "
                    "with -stop-after, -stop-before"));

In any case, notice that the current patch has a missing space before -stop-after in the string.

493 ↗(On Diff #201684)

Remove the extra blank line.

llvm/lib/Target/PowerPC/PPC.h
91 ↗(On Diff #201684)

This looks like a drive-by fix. If it is not, then clarify the relation of this comment to AIX.

llvm/lib/Target/PowerPC/PPCFastISel.cpp
1967 ↗(On Diff #201684)

See the comments I made on FastISel.cpp.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
4416 ↗(On Diff #201684)

Using dyn_cast and then dereferencing the result without checking for null is not right. If the cast is expected to succeed, use cast. In that case, please also add a comment to explain why the cast is expected to succeed.

4419 ↗(On Diff #201684)

Add a comma before "then".

4918 ↗(On Diff #201684)

s/functions/function's/;

4919 ↗(On Diff #201684)

s/pre-pending a "." to/inserting a "." before/;
s/functions/function's/;

4923 ↗(On Diff #201684)

dyn_cast may return a null pointer. Please use cast if you expect the cast to succeed.

syzaara added inline comments.Jun 4 2019, 9:59 AM
llvm/lib/CodeGen/TargetPassConfig.cpp
204 ↗(On Diff #201684)

nit: Init -> Initialize
End sentence with period.

llvm/lib/CodeGen/TargetPassConfig.cpp
204 ↗(On Diff #201684)

To be contextually consistent with the surrounding use of cl::desc, the string should not end in a period.

sfertile added inline comments.Jun 4 2019, 10:54 AM
llvm/lib/CodeGen/LLVMTargetMachine.cpp
204 ↗(On Diff #201684)

We either need to:

  1. initialize the MCObjectFileInfo to be able to use mir tests with call instructions on AIX
  2. Be able to add the requisite info the the MCSymbol base class, and not need to cast callee operands to an MCSymbolXCOFF during lowering.

If its safe to initialize MCObjectFileInfo even when we don't have an AsmPrinter, then maybe we should just always do so for MIR output on AIX.

llvm/lib/Target/PowerPC/PPC.h
91 ↗(On Diff #201684)

This is my fault, I suggested we needed to fix comment but failed to explain it should be in a separate NFC patch. I'll land this separately for you.

Xiangling_L marked 15 inline comments as done.
Xiangling_L edited the summary of this revision. (Show Details)

Addressed 1st round reviews.

jasonliu added inline comments.Jun 4 2019, 2:20 PM
llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
1878 ↗(On Diff #201684)

I vaguely remember the discussion being... we will disable this fast-isel path for now on AIX and get the non-fast-isel path working first. We would deal with fast-isel path later down the road.
If my memory is correct, then I don't think it is a "permanent fact". So maybe we don't want to use words like "should always", and state that this would be "for now" and we will evaluate the situation later.

llvm/lib/Target/PowerPC/PPC.h
92 ↗(On Diff #203018)

Extra blank line?

Xiangling_L marked an inline comment as done.Jun 4 2019, 2:33 PM
Xiangling_L added inline comments.
llvm/lib/CodeGen/LLVMTargetMachine.cpp
204 ↗(On Diff #201684)

Some concerns as I discussed with Sean:

The base class implementation is not a pure virtual:

virtual TargetLoweringObjectFile *getObjFileLowering() const {
    return nullptr;
  }
not sure if something like that is expected to have a non-null TLOF.

Put some of my thoughts about doing target lowering object file code here:

[line 206]const_cast<TargetLoweringObjectFile&(*this->getObjFileLowering())
          .Initialize(Ctx, *this);
is extracted from generic AsmPrinter::doInitialization (lib/CodeGen/AsmPrinter/AsmPrinter.cpp:258), which means any backend that can init an AsmPrinter will have a non-null TLOF for sure, given that it seems all xxxAsmPrinter::doInitialization will finally call AsmPrinter::doInitialization. And back to our case here, it looks like the backends which will go through this path are only those who can add an AsmPrinter pass to, so I think it won't have a non-null TLOF.
Xiangling_L marked 2 inline comments as done.Jun 4 2019, 2:38 PM
Xiangling_L added inline comments.
llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
1878 ↗(On Diff #201684)

If we don't always ban fast-isel of call loweing on AIX, does that mean we need to duplicate logic of creating MCSymbolSDNode/MCSymbolXCOFF for function entry point on fast-isel path?

llvm/lib/Target/PowerPC/PPC.h
92 ↗(On Diff #203018)

Thanks. will address this in the next update.

llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
1878 ↗(On Diff #201684)

I agree with Jason. There should be something about how the fast-isel path does not do the mapping at this time, and the "can be mapped" should say "will be mapped" to avoid the connotation that doing the mapping cannot be done other than by going through DAG-ISEL.

llvm/lib/Target/PowerPC/PPCFastISel.cpp
1967 ↗(On Diff #201684)

Make the same tweaks to the comment here.

Xiangling_L marked 4 inline comments as done.

address 2nd reviews;

  1. remove extra blank lines
  2. tweak comments for disabling aix call lowering fast-isel path
  3. set "always do target lowering object file" for AIX
llvm/lib/CodeGen/TargetPassConfig.cpp
490 ↗(On Diff #203168)

Should we explain the rationale here?

491 ↗(On Diff #203168)

Remove the extra space after getTargetTriple.

llvm/test/CodeGen/PowerPC/test_call_aix.ll
1 ↗(On Diff #203168)

The -init-MC is no longer needed now; right?

Xiangling_L marked 3 inline comments as done.Jun 5 2019, 12:28 PM
Xiangling_L added inline comments.
llvm/test/CodeGen/PowerPC/test_call_aix.ll
1 ↗(On Diff #203168)

You are right. And I am wondering do we still wanna create this option for other platforms? or do we wanna make some changes to do target object file lowering for AIX only?

llvm/test/CodeGen/PowerPC/test_call_aix.ll
1 ↗(On Diff #203168)

I spoke with @sfertile today, and my understanding is that we don't want to add the option if we will just always perform the initialization on AIX (in which case, we don't need the willInitMCObjectFile function either).

Xiangling_L marked 2 inline comments as done.Jun 5 2019, 6:26 PM

3rd: delete init-MC option, and enable lowering object file on AIX always for MIR output.

Thanks Xiangling. Just a further minor comment.

llvm/lib/CodeGen/LLVMTargetMachine.cpp
206 ↗(On Diff #203278)

Typo: "calle".
s/And to/To/;

Xiangling_L marked 3 inline comments as done.

address minor changes

I think everyone's comments have been addressed. Thanks again, Xiangling.

llvm/lib/CodeGen/LLVMTargetMachine.cpp
205 ↗(On Diff #203361)

Sorry for missing this earlier. Please remove the space before the comma.

Xiangling_L marked an inline comment as done.

rebase onto the latest master branch and address minor changes

sfertile accepted this revision.Jun 6 2019, 9:40 AM

LGTM.

llvm/lib/CodeGen/LLVMTargetMachine.cpp
205 ↗(On Diff #203361)

I would suggest slightly rewording thjis comment along the lines of:

On AIX we might manifest MCSymbols during SDAG lowering. For mir testing to be meaningful we need to ensure that the symbols created are MCSymboLXCOFF variants, which requires that TargetLoweringObjectFile has been initialized.
This revision is now accepted and ready to land.Jun 6 2019, 9:40 AM

Fix LIT testcase from delivered call lowering patch.

address minor changes

llvm/lib/CodeGen/LLVMTargetMachine.cpp
205 ↗(On Diff #203406)

Add a comma after "AIX".

206 ↗(On Diff #203406)

Add a comma after "meaningful".

207 ↗(On Diff #203406)

s/MCSymboLXCOFF/MCSymbolXCOFF/;

208 ↗(On Diff #203406)

s/TargetLoweringObjectFile/the TargetLoweringObjectFile instance/;

Last few comments by Hubert addressed in the landed commit.

This revision was automatically updated to reflect the committed changes.
gribozavr added inline comments.
llvm/trunk/include/llvm/MC/MCSymbolXCOFF.h
13

llvm/IR/GlobalValue.h can't be included in MC, that creates a circular dependency between MC and IR libraries. This circular dependency is causing an issue for build system that enforce layering. Is there another way to implement this change?

gribozavr added inline comments.Jun 7 2019, 2:25 AM
llvm/trunk/include/llvm/MC/MCSymbolXCOFF.h
13

I committed a workaround in r362782, but it is only a workaround, and it would be nice to have a real fix.

Xiangling_L marked 2 inline comments as done.Jun 7 2019, 8:44 AM
Xiangling_L added inline comments.
llvm/trunk/include/llvm/MC/MCSymbolXCOFF.h
13

Thanks for the workaround. We will discuss and see if there is a better way to implement this.

Xiangling_L marked an inline comment as done.Jul 7 2019, 6:16 AM
Xiangling_L added inline comments.
llvm/trunk/include/llvm/MC/MCSymbolXCOFF.h
13

Hi, I am now working on a fix for this. And I am wondering if there is any documentation about things like IR&MC dependency you pointed out here?