This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC][PCRelative] Thread Local Storage Support for Initial Exec
ClosedPublic

Authored by kamaub on Jun 16 2020, 9:43 AM.

Details

Summary

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.

Diff Detail

Event Timeline

kamaub created this revision.Jun 16 2020, 9:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 16 2020, 9:43 AM
kamaub added a reviewer: Restricted Project.Jun 16 2020, 9:47 AM
kamaub added a project: Restricted Project.

Overall this seems fine. I have a number of comments but they are mostly minor.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCCodeEmitter.cpp
333

nit:
isPCRel -> IsPCRel

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

Why 32 and not 16?

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

nit:
isPCRel -> IsPCRel

3041

The only difference between the two global addresses is the flags that are added in the PCRel case.
It may be clearer to write it as: (ignore my spacing)

    SDValue TGA = DAG.getTargetGlobalAddress(GV, dl, PtrVT, 0,
	​                                     isPCRel ? (PPCII::MO_PCREL_FLAG | PPCII::MO_TPREL_FLAG)  : 0)
3045

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

nit:
Should probably use pwr10 instead of future in both places.

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
114

16 is used for MO_TLSGD_FLAG = 16 in https://reviews.llvm.org/D82315
We originally planned to set 32 here for the convenience of code merging.

kamaub updated this revision to Diff 272778.Jun 23 2020, 11:13 AM

Addressing review comments.

kamaub updated this revision to Diff 273768.Jun 26 2020, 9:56 AM
  • 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.
kamaub marked 6 inline comments as done.Jun 26 2020, 10:04 AM

I think this patch is getting close. A couple more comments from me.

llvm/lib/Target/PowerPC/PPCMCInstLower.cpp
90

I would move the setting of VK_PPC_TLS_PCREL into the switch above.
The PPCII::MO_TLS is not really a flag and with the current code we set RefKind = MCSymbolRefExpr::VK_PPC_TLS and then we set it to MCSymbolRefExpr::VK_PPC_TLS_PCREL afterward.

llvm/test/CodeGen/PowerPC/pcrel-tls-initial-exec.ll
6 ↗(On Diff #273768)

nit:
Use mcpu=pwr10

kamaub updated this revision to Diff 276031.Jul 7 2020, 6:10 AM
  • 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.
kamaub marked 2 inline comments as done.Jul 7 2020, 8:39 AM

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
113

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
13 ↗(On Diff #276031)

nit: change to InitialExecAddressLoad to better describe the case

23 ↗(On Diff #276031)

Please add a comment here for the one-byte displacement. Same for line 39.

29 ↗(On Diff #276031)

nit: change to InitialExecValueLoad

llvm/test/MC/PowerPC/pcrel-tls-initial-exec-address-load-reloc.s
7 ↗(On Diff #276031)

R_PPC64_TLS (with one-byte displacement)

15 ↗(On Diff #276031)
  • You can only keep necessary assembly in the test and remove the comments.
  • Rename the function.
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
7 ↗(On Diff #276031)

ditto

21 ↗(On Diff #276031)

ditto

kamaub updated this revision to Diff 283306.Aug 5 2020, 11:08 AM

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
kamaub marked 5 inline comments as done.Aug 5 2020, 11:11 AM
kamaub marked 3 inline comments as done.Aug 5 2020, 1:47 PM
kamaub updated this revision to Diff 283895.Aug 7 2020, 6:39 AM

Rebasing and updating patch to use TLS enablement option.

stefanp accepted this revision as: stefanp.Aug 20 2020, 2:00 PM

Couple of nits.
LGTM

llvm/test/MC/PowerPC/pcrel-tls-initial-exec-address-load-reloc.s
18 ↗(On Diff #283895)

nit:
You can delete the comment (# @InitialExec) here and just leave the label.

llvm/test/MC/PowerPC/pcrel-tls-initial-exec-value-load-reloc.s
18 ↗(On Diff #283895)

nit:
You can delete the comment and just leave the label.

This revision is now accepted and ready to land.Aug 20 2020, 2:00 PM
This revision was landed with ongoing or failed builds.Aug 21 2020, 8:13 AM
This revision was automatically updated to reflect the committed changes.
kamaub marked 2 inline comments as done.Aug 21 2020, 8:14 AM