This is an archive of the discontinued LLVM Phabricator instance.

[AIX][TLS] Generate 32-bit local-exec access code sequence
ClosedPublic

Authored by amyk on Jun 11 2023, 8:41 PM.

Details

Summary

This patch adds support for the TLS local-exec access model on AIX to allow
for the ability to generate the 32-bit (specifically, non-optimized) code sequence.
This work is a follow up of D149722.

The particular sequence that is generated for this sequence is as follows:

.tc var[TC],var[TL]@le.   // variable offset, with the le relocation specifier

bla .__get_tpointer()     // get the thread pointer, modifies r3
lwz reg1, var[TC](2)      // load the variable offset
add reg2, r3, reg1        // add the variable offset to the retrieved thread pointer

Diff Detail

Event Timeline

amyk created this revision.Jun 11 2023, 8:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 11 2023, 8:41 PM
amyk requested review of this revision.Jun 11 2023, 8:41 PM
amyk edited the summary of this revision. (Show Details)Jun 12 2023, 6:16 AM
amyk updated this revision to Diff 530976.Jun 13 2023, 10:27 AM
  • Rebase patch.
  • Update patch comments.

First review, I'll try to comeback again soon.

llvm/lib/Target/PowerPC/PPCTLSDynamicCall.cpp
68

for me personally !IsTLSTPRelMI less confusing.

150

contents need to be indented

amyk added inline comments.Jun 14 2023, 11:40 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
1741

Two PPCISD nodes may be unnecessary.

3346

Move 3356-3358 to the top before the checks for 64-bit/32-bit.

llvm/lib/Target/PowerPC/PPCInstrInfo.td
3156

Double check if these defines are correct.

amyk marked 2 inline comments as done.Jun 15 2023, 1:37 PM
amyk added inline comments.
llvm/lib/Target/PowerPC/PPCInstrInfo.td
3156

We should just list LR, and potentially R3.
I don't believe R0 is needed here.

llvm/lib/Target/PowerPC/PPCTLSDynamicCall.cpp
68

I'll update this, thank you.

amyk updated this revision to Diff 531893.Jun 15 2023, 1:43 PM
amyk marked an inline comment as done.

Address review comments:

  • Update comments
  • Remove unnecessary PPCISD node
  • Confirm clobbers list
lei accepted this revision as: lei.Jun 16 2023, 8:25 AM
lei added a subscriber: lei.

Just a minor comment otherwise LGTM.
Thx

llvm/lib/Target/PowerPC/PPCTLSDynamicCall.cpp
88–89

Since I believe the only time we want to override InReg is if both of those conditions are true?

This revision is now accepted and ready to land.Jun 16 2023, 8:25 AM
amyk updated this revision to Diff 532365.Jun 16 2023, 10:37 PM
amyk marked an inline comment as done.
  • Address Lei's comment in fixing the condition in PPCTLSDynamicCall.cpp
  • Apply the same relocation/symbol simplification checks as in D149722 in the relocation LIT tests.
nemanjai accepted this revision.Jun 19 2023, 8:41 AM

LGTM aside from some minor nits.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
622–632

Can this not be something like:

StringRef SymName = MIOpc == PPC::GETtlsTpointer32AIX ?
  ".__get_tpointer" : ".__tls_get_addr";

return Ctx.getXCOFFSection(SymName, SectionKind::getText(),
          XCOFF::CsectProperties(XCOFF::XMC_PR, XCOFF::XTY_ER))
  ->getQualNameSymbol();
634

Can this not be a static file-scope function?

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

Not sure if it was clang-format that suggested this, but please put these on the same line so that it matches the surrounding formatting.

llvm/lib/Target/PowerPC/PPCTLSDynamicCall.cpp
59

Please add a comment since I don't think the name of the variable can adequately represent what it is for. Something like:

// There are a number of slight differences in code generation
// when we call .__get_tpointer (32-bit AIX TLS).
160

Nit: please refrain from putting single statements in braces.

amyk marked 5 inline comments as done.Jun 19 2023, 2:05 PM
amyk added inline comments.
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
622–632

Thanks for the suggestion.

634

I like this suggestion and I looked into this. I believe it cannot as I need to use EmitToStreamer(), however I could be wrong/missing something.

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

Yeah, this was the result of clang-format. I'll move this line back.

llvm/lib/Target/PowerPC/PPCTLSDynamicCall.cpp
59

Comment added.

160

Good catch. I did this also in PPCISelLowering.cpp, so I'll remove those accordingly.

amyk updated this revision to Diff 532749.Jun 19 2023, 2:06 PM
amyk marked 5 inline comments as done.

Add comments, and simplify createMCSymbolForTlsGetAddr().

amyk updated this revision to Diff 532899.Jun 20 2023, 6:50 AM

Simplify relocation test cases.

This revision was landed with ongoing or failed builds.Jun 20 2023, 9:59 AM
This revision was automatically updated to reflect the committed changes.