This is an archive of the discontinued LLVM Phabricator instance.

[AIX][TLS] Add ASM portion changes to support TLSGD relocations to XCOFF objects
ClosedPublic

Authored by NeHuang on Apr 21 2021, 6:52 AM.

Details

Summary

This patch adds ASM portion changes to support TLSGD relocations to XCOFF objects:

  • Add new variantKinds for the symbol's variable offset and region handle
  • Print the proper relocation specifier @gd in the asm streamer when emitting the TC Entry for the variable offset for the symbol
  • Fix the switch section failure between the TC Entry of variable offset and region handle
  • Put .__tls_get_addr symbol in the ProgramCodeSects with XTY_ER property

Diff Detail

Event Timeline

NeHuang created this revision.Apr 21 2021, 6:52 AM
NeHuang requested review of this revision.Apr 21 2021, 6:52 AM
sfertile added inline comments.Apr 22 2021, 10:41 AM
llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp
141

Real minot nit: I think it might be a bit clearer to break this comment into 2 separate sentences, one for the region handle and its variant-kind/relocation-specifier and one for the variable offset and its variant-kind/relocation-specifier.

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

Its kind of odd to create this symbol just to end up creating the proper symbol for AIX later in the AIX specific handling. In the existing code its a little sloppy but the second call to OutContext.getOrCreateSymbol(Name); at least looks up the same symbol created here. When we create the csect and get its qualname symbol, is that the same MCSymbol as created on this line?

My suggestion is we move creating this symbol down to just after the AIX specific code returns (and if we end up creating the helper function for creating the qual-name symbol for .__tls_get_addr then the name shouldn't be need here either and can also be moved).

561

We have this same block of code twice in this file (here and line 2320), maybe is it worthwhile to move it to a descriptively named helper function?

sfertile added inline comments.Apr 22 2021, 12:17 PM
llvm/test/CodeGen/PowerPC/aix-tls-gd-double.ll
29

This emission of the call instructions to tls_get_addr change because we now create the csect and use the qual-name symbol instead of creating just the external symbol, but we have no test changes that show this behaviour change. We can either update these tests to show the change, or recommit a smaller more focused lit test and show the change with that.

NeHuang updated this revision to Diff 340655.Apr 26 2021, 2:41 PM

Address review comment to

  • Create a helper function createMCSymbolForTlsGetAddr and merge unnecessary code
  • Split the comments in PPCMCTargetDesc.cpp
  • Update the existing test cases to reflect the changes when emitting tls_get_addr
sfertile accepted this revision as: sfertile.Apr 28 2021, 8:17 AM

One minor nit but otherwise LGTM.

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

Real minor nit: Its probably better the keep the helper as AIX only and then we don't need to worry about the boolean argument.

This revision is now accepted and ready to land.Apr 28 2021, 8:17 AM
NeHuang updated this revision to Diff 341281.Apr 28 2021, 11:52 AM
  • Addressed review comment to create the MCsymbol for AIX only in the helper function.
  • Rebased the patch with ToT.