This patch is the initial support for the Intial Exec Thread Local
Local Storage model to produce code sequence and relocations correct
to the ABI for the model when using PC relative memory operations.
Details
- Reviewers
stefanp nemanjai NeHuang - Group Reviewers
Restricted Project - Commits
- rG365f861c45bb: [PowerPC][PCRelative] Thread Local Storage Support for Initial Exec
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Overall this seems fine. I have a number of comments but they are mostly minor.
llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCCodeEmitter.cpp | ||
---|---|---|
354 | nit: | |
llvm/lib/Target/PowerPC/PPC.h | ||
112 | Why 32 and not 16? | |
llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
2981–2991 | nit: | |
2985 | The only difference between the two global addresses is the flags that are added in the PCRel case. SDValue TGA = DAG.getTargetGlobalAddress(GV, dl, PtrVT, 0, isPCRel ? (PPCII::MO_PCREL_FLAG | PPCII::MO_TPREL_FLAG) : 0) | |
2989 | Same thing here: SDValue TGATLS = DAG.getTargetGlobalAddress(GV, dl, PtrVT, 0, isPCRel ? PPCII::MO_TLS | PPCII::MO_PCREL_FLAG : PPCII::MO_TLS) | |
llvm/test/CodeGen/PowerPC/pcrel-tls-inital-exec.ll | ||
6 ↗ | (On Diff #271125) | nit: |
Overall looks good to me.
Would be better to add tests for assembly reader check in llvm/test/MC/PowerPC
llvm/lib/Target/PowerPC/PPC.h | ||
---|---|---|
112 | 16 is used for MO_TLSGD_FLAG = 16 in https://reviews.llvm.org/D82315 |
- Rebasing patch to be dependant on D82315 - TLS Support for General Dynamic.
- Adding Combination flag MO_GOT_TPREL_PCREL_FLAG to more explicitly define the king that will output @got@tprel@pcrel relocation.
- Adding assembly reader test cases by request of reviewer for more robust testing.
- Minor format changes.
I think this patch is getting close. A couple more comments from me.
llvm/lib/Target/PowerPC/PPCMCInstLower.cpp | ||
---|---|---|
94 | I would move the setting of VK_PPC_TLS_PCREL into the switch above. | |
llvm/test/CodeGen/PowerPC/pcrel-tls-initial-exec.ll | ||
7 | nit: |
- rearranging conditional logic for emitting MCSymbolRefExpr::VK_PPC_TLS_PCREL so that it is less redundant and reads better.
- replacing future with pwr10 in test cases and adding better comments.
Overall looks fine to me, but of course please wait to hear from Stefan and Nemanja.
Some nits added.
llvm/lib/Target/PowerPC/PPC.h | ||
---|---|---|
128 | nit: use MO_GOT_FLAG | MO_TPREL_FLAG | MO_PCREL_FLAG to be consistent with order in relocation name and the flag name. You can also update it for MO_GOT_TLSGD_PCREL_FLAG | |
llvm/test/CodeGen/PowerPC/pcrel-tls-initial-exec.ll | ||
14 | nit: change to InitialExecAddressLoad to better describe the case | |
24 | Please add a comment here for the one-byte displacement. Same for line 39. | |
30 | nit: change to InitialExecValueLoad | |
llvm/test/MC/PowerPC/pcrel-tls-initial-exec-address-load-reloc.s | ||
8 | R_PPC64_TLS (with one-byte displacement) | |
16 |
InitialExecAddressLoad: pld 3, x@got@tprel@pcrel(0), 1 add 3, 3, x@tls@pcrel blr | |
llvm/test/MC/PowerPC/pcrel-tls-initial-exec-value-load-reloc.s | ||
8 | ditto | |
22 | ditto |
Rebasing and addressing review comments:
- rebased on the updated version of D82315
- updated test cases according to reviews
- updated llvm-readobj run lines in test cases to reflect interface change
nit:
isPCRel -> IsPCRel