This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Support initial-exec TLS relocation on AIX
ClosedPublic

Authored by qiucf on Jul 25 2023, 10:06 PM.

Details

Summary

This patch adds TLS_IE relocation type to XCOFF writer, and emit code sequence for initial-exec TLS variables.

Diff Detail

Event Timeline

qiucf created this revision.Jul 25 2023, 10:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 25 2023, 10:06 PM
qiucf requested review of this revision.Jul 25 2023, 10:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 25 2023, 10:06 PM
qiucf retitled this revision from [PowerPC] Support initial-exec TLS mode on AIX to [PowerPC] Support initial-exec TLS relocation on AIX.Jul 25 2023, 10:06 PM
lkail added a subscriber: lkail.Aug 1 2023, 10:17 PM
lkail added inline comments.
llvm/test/CodeGen/PowerPC/aix-tls-ie-ldst.ll
1558

Are these CHECK lines generated by the script? If not, better remove the head line.

@amyk @hubert.reinterpretcast , do you guys thinks it is ok to make initial exec mode have same code sequences with local exec mode? Seems OK to me.

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

nit: no need for the else

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

Nit: update the comments here. Now we support 3 modes.

llvm/test/CodeGen/PowerPC/aix-tls-ie-xcoff-reloc.ll
215

It may be also necessary to check the relocations in the text section. We want to make sure we have call to .__get_tpointer at 32-bit.

qiucf updated this revision to Diff 547642.Aug 6 2023, 9:38 PM
qiucf marked 4 inline comments as done.

It is not clear how someone in the future should react if the test case changes behaviour. You should think about maintainability of the test cases. It would appear that a part of the checks were produced using a script and a part was added manually. As such, it is not clear what the essential parts of the tests are so anyone maintaining this in the future must really dive deep into the TLS implementation to understand if changes are safe or not. I'm not sure what the best approach would be. Perhaps add a comment describing what is being tested. Or maybe only test the key parts using manual checks.

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

This message should probably be updated.

qiucf updated this revision to Diff 548083.Aug 8 2023, 12:03 AM
qiucf marked an inline comment as done.
lkail added inline comments.Aug 8 2023, 1:52 AM
llvm/test/CodeGen/PowerPC/aix-tls-ie-ldst.ll
1561

You may add checks of labels L..Cx rather than simply counting number of TOC entries. We should care about each TLV and its corresponding relocation expressions.

lkail added inline comments.Aug 8 2023, 2:04 AM
llvm/test/CodeGen/PowerPC/aix-tls-ie-ldst.ll
1561
; CHECK-LABEL:    .toc                                                                                                                                        
; CHECK: L..C0:                                                                                                                                               
; CHECK-NEXT:   .tc global_int_zero[TE],global_int_zero[TL]@ie                                                                                                
...
qiucf updated this revision to Diff 550632.Aug 15 2023, 11:46 PM
qiucf marked 2 inline comments as done.
lkail accepted this revision as: lkail.Aug 22 2023, 12:27 AM

LGTM.

This revision is now accepted and ready to land.Aug 22 2023, 12:27 AM
This revision was automatically updated to reflect the committed changes.