This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by amyk on May 2 2023, 10:07 PM.

Details

Summary

This patch adds support for the TLS local-exec access model on AIX to allow
for the ability to generate the 64-bit (specifically, non-optimized) code sequence.

For this patch in particular, the sequence that is generated involves a load of the
variable offset, followed by an add of the loaded variable offset to r13 (which is
thread pointer, respectively). This code sequence looks like the following:

ld reg1,var[TC](2)
add reg2, reg1, r13     // r13 contains the thread pointer

The TOC (.tc pseudo-op) entries generated in the assembly files are also
changed where we add the @le relocation for the variable offset.

Diff Detail

Event Timeline

amyk created this revision.May 2 2023, 10:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2023, 10:07 PM
amyk requested review of this revision.May 2 2023, 10:07 PM
amyk updated this revision to Diff 520593.May 8 2023, 10:48 PM

Fix issue with respect to generating the incorrect code for the large code model.

amyk updated this revision to Diff 521704.May 13 2023, 10:28 PM

Rebase patch and add test case for floats.

amyk updated this revision to Diff 526051.May 26 2023, 6:45 AM

Rebase and pinging this patch for review.

amyk edited the summary of this revision. (Show Details)Jun 1 2023, 8:40 AM
kamaub added a comment.Jun 2 2023, 7:46 AM

Thank you for this patch, just some review comments.

llvm/lib/Target/PowerPC/PPC.h
130–132

Accidentally repeated the initial comment?
What is the difference between TLS Initial Exec model and Initial Exec models on AIX?

Follow up, I wondering since it is named TPREL to mean Thread pointer relative, which indicates using r13 instead of the get addr call like with dynamic models should we say something like:

/// MO_TPREL_FLAG - If this bit is set the symbol reference is relative to
/// the thread pointer and is used for TLS Initial Exec , or  TLS Local Exec model.
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
816

I think we should expand to say local exec explicitly

823

inline this variable

825

Please add an llvm unreachable after this if

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
3355–3358

I was wanted to suggest a wording that states the key points more explicitly

llvm/lib/Target/PowerPC/PPCInstr64Bit.td
1924

Should this have an IsAIX guard?

llvm/test/CodeGen/PowerPC/aix-tls-le-xcoff-reloc-large.ll
2

Please change the xcoff test cases to pwr7 for testing consistency

amyk edited the summary of this revision. (Show Details)Jun 3 2023, 8:01 AM
amyk updated this revision to Diff 528186.Jun 3 2023, 9:42 PM
amyk marked 6 inline comments as done.
  • Updated documentation for local-exec 64-bit in PPCISelLoweing.cpp, and for the ADD_TLS node, etc
  • Remove assert(), add llvm_unreachable() and report_fatal_error() calls
  • Simplified some code
llvm/lib/Target/PowerPC/PPC.h
130–132

Thanks, Kamau. I've updated this comment.

For initial-exec on AIX, it appears that the regular access sequence looks the same as the local-exec sequence, with the exception that we have a specific the initial exec relocation specifier (ie) for the TLS data in the TOC.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
3336
  • Add documentation on what we're trying to do/produce for local-exec and the code sequence.
  • Add the local exec code sequence in the patch description.
  • Update the comment for ADD_TLS in PPCISelLowering.h as this node is not only used for initial-exec.
3345

Suggestion:

  • Remove assert().
  • Add an else to handle the 32-bit behaviour for now. We can report_fatal_error() for 32-bit mode.
  • Remove the TODO for 32-bit.
amyk marked an inline comment as done.Jun 6 2023, 11:40 AM

Leaving comments from a group code review.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp
112

Verify if this change is appropriate for SignAndSizeForFKData.

llvm/lib/Target/PowerPC/PPC.h
132

Missing an and in this comment.

llvm/lib/Target/PowerPC/PPCInstr64Bit.td
1911
  • Clean up comments to combine both this pattern and the one below.
  • Expand this comment to say that it's used for global-dynamic, or loading for local-exec (offset for the TLS variable is in the TOC)
  • Remove code model comments from the PPCaddTls.
1916
  • Remove this comment, as it is not necessary to explain the small/large code model intricacies here.
  • Add the comment on why we need ADDTLS (in order to add the offset to r13, and in case we can optimized the user of the address).
1924

We've discussed this - having a guard specifically for AIX should not be necessary as producing the PPCaddTls node with two i64 inputs is only possible on AIX.

DiggerLin added inline comments.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
3334

TM only use once, change the above code to

TLSModel::Model Model = getTargetMachine().getTLSModel(GV); ?

amyk updated this revision to Diff 530975.Jun 13 2023, 10:26 AM
amyk marked 2 inline comments as done.
  • Rebase patch.
  • Update comments.
  • Remove the target machine variable as per Digger's comments.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
3334

Thanks for the suggestion. I've incorporated this into the patch.

amyk added inline comments.Jun 15 2023, 12:12 PM
llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp
112

I talked to @DiggerLin and I think this change is appropriate and should be something we expect.

For the TLS relocations, we don't expect them to have the sign bit set (and XL doesn't appear to set them for the R_TLS* relocations either). The SignAndSizeForFKData should correspond to the relocations that we emit in the llvm-readobj --relocs --expand-relocs output for IsSigned and Length:

; RELOC-NEXT:     Relocation {
; RELOC-NEXT:       Virtual Address: 0x68
; RELOC-NEXT:       Symbol: IThreadLocalVarUninit (27)
; RELOC-NEXT:       IsSigned: No
; RELOC-NEXT:       FixupBitValue: 0
; RELOC-NEXT:       Length: 64
; RELOC-NEXT:       Type: R_TLS_LE (0x23)
; RELOC-NEXT:     }

like in these tests:

llvm/test/CodeGen/PowerPC/aix-tls-le-xcoff-reloc-large.ll
llvm/test/CodeGen/PowerPC/aix-tls-le-xcoff-reloc.ll

The IsSigned and Length values appear correct and match what XL emits.

Also, according to the AIX documentation for the Relocation Information for XCOFF File, it states that the relocation sign and size contents:

0x80 (1 bit) - Indicates whether the relocation reference is signed (1) or unsigned (0).
0x40 (1 bit) - If this field is one, it indicates that the binder replaced the original instruction with a modified instruction.
0x3F(6 bits) - Specifies the bit length of the relocatable reference minus one. The current architecture allows for fields of up to 32 bits (XCOFF32) or 64 bits (XCOFF64) to be relocated.

I believe this is mainly consistent to what is currently implemented in the file - we are not using IsSigned for the R_TLS_LE relocation, and we are ensuring the length is 32 for XCOFF32, and 64 for XCOFF64, like what XL does.

lei added a subscriber: lei.Jun 16 2023, 8:06 AM
lei added inline comments.
llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp
155

These comments seem to be something appeneded each time a new condtion was added. Can we clean up lines 150-155 to be more general and representative? eg

// If the variant kind is a type of VK_PPC_AIX_TLS*, the entry represents the region handle for the symbol and
// we add the corresponding relocation specifier @[m|gd|le].
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
824

I dont' think we need the tmp variable Model, can just update the if stmt thus:

if (TM.getTLSModel(MO.getGlobal() == TLSModel::LocalExec)
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
3355–3357

I don't think Model is used anywhere else... can probably inline that as well.

3356

Is64Bit is only used once. Please inline and remove this variable def.

lei added inline comments.Jun 16 2023, 8:08 AM
llvm/include/llvm/MC/MCExpr.h
302

I realize this was done similar to VK_PPC_AIX_TLS*. but the symbol outputed seems to be too general and feels like we should fix this. Shouldn't this be something more along the lines of symbol@aix@tlsle?

nemanjai accepted this revision.Jun 16 2023, 8:50 AM

LGTM.

llvm/include/llvm/MC/MCExpr.h
302

These are actual strings printed in ASM (i.e. the textual representation of the relocation) so we can't freely add aix in there since the assembler would not know what to do with that.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp
155

I think it makes sense to unify the gd/le, but m is the region handle while the other two are variable offsets. Or we can simply state something like:

// On AIX, we have a region handle (symbol@m) and the variable offset
// (symbol@{gd|le}) for TLS variables, depending on the TLS model.

The details for which is which should be obvious from the code.

This revision is now accepted and ready to land.Jun 16 2023, 8:50 AM
lei accepted this revision.Jun 16 2023, 8:56 AM

LGTM

llvm/include/llvm/MC/MCExpr.h
302

I see. Thank-you.

amyk marked 6 inline comments as done.Jun 16 2023, 9:24 AM
amyk added inline comments.
llvm/include/llvm/MC/MCExpr.h
302

That's correct. Thanks for answering this question, Nemanja.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp
155

Good suggestion. I'll update the patch to reflect this.

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

My initial plan was to introduce these variables to be used for later when we implement the other models.
I'll be more explicit about this next time (especially if I have a future plan on why I added these variables).

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
3355–3357

My initial plan was to introduce these variables to be used for later when we implement the other models.
I'll be more explicit about this next time (especially if I have a future plan on why I added these variables).

3356

My initial plan was to introduce these variables to be used for later when we implement the other models.
I'll be more explicit about this next time (especially if I have a future plan on why I added these variables).

DiggerLin added inline comments.Jun 16 2023, 11:40 AM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
824
TLSModel::Model Model = TM.getTLSModel(MO.getGlobal());
 if (Model == TLSModel::LocalExec)

change to

if(TM.getTLSModel(MO.getGlobal() == TLSModel::LocalExec))
DiggerLin added inline comments.Jun 16 2023, 11:52 AM
llvm/test/CodeGen/PowerPC/aix-tls-le-xcoff-reloc-large.ll
151

in order to make the test case more readable, suggest to remove all not related the local_exec TLS function CHECK.

275

ditto

llvm/test/CodeGen/PowerPC/aix-tls-le-xcoff-reloc.ll
177

ditto

194

ditto

amyk marked 7 inline comments as done.Jun 16 2023, 12:03 PM
amyk added inline comments.
llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp
155

I like this suggestion. I'll update the comment accordingly.

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

Discussed this with Digger.
Digger is OK if we keep this variable, as I have justified above on why I added it.

DiggerLin added inline comments.Jun 16 2023, 12:34 PM
llvm/test/CodeGen/PowerPC/aix-tls-le-xcoff-reloc.ll
177

sorry for this one, we need to keep this one.

amyk updated this revision to Diff 532364.Jun 16 2023, 10:26 PM
amyk marked 6 inline comments as done.
  • Update comments.
  • Address Digger's comments in simplifying the RELOC and SYM checks in the relocation LIT test cases.
This revision was automatically updated to reflect the committed changes.
llvm/test/CodeGen/PowerPC/aix-tls-le-xcoff-reloc.ll
8

It helps to test with more than variable in .tdata (or in .tbss).

Defect opened:
https://github.com/llvm/llvm-project/issues/63885

amyk added inline comments.
llvm/test/CodeGen/PowerPC/aix-tls-le-xcoff-reloc.ll
8

I posted https://reviews.llvm.org/D155415 in hopes in addressing this.