This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC][PCRelative] Thread Local Storage Support for General Dynamic
ClosedPublic

Authored by kamaub on Jun 22 2020, 9:46 AM.

Details

Summary

This patch is the initial support for the General Dynamic Thread Local
Local Storage model to produce code sequence and relocations correct
to the ABI for the model when using PC relative memory operations.

Patch by: NeHuang

Diff Detail

Event Timeline

kamaub created this revision.Jun 22 2020, 9:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 22 2020, 9:46 AM
kamaub added a reviewer: Restricted Project.Jun 22 2020, 9:47 AM
kamaub added a project: Restricted Project.
kamaub updated this revision to Diff 272478.Jun 22 2020, 9:49 AM

Updating patch with author's credits.

kamaub edited the summary of this revision. (Show Details)Jun 22 2020, 9:50 AM
Harbormaster completed remote builds in B61260: Diff 272477.
kamaub updated this revision to Diff 273480.Jun 25 2020, 12:22 PM

Adding a combination flag to more explicitly set the bits needed to produce
General Dynamic TLS model relocations.

kamaub updated this revision to Diff 273481.Jun 25 2020, 12:28 PM

Addressing clang format suggestion.

Bunch of nits but overall the idea is fine. I only had one real concern with respect to the setup of InReg and you can find that in the comments.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCInstPrinter.cpp
535

nit:
That comment is confusing a couple of things.
The reason we have to do all of this special handling is because we don't want:

__tls_get_addr(x@tlsgd)@notoc

We want:

__tls_get_addr@notoc(x@tlsgd)

Maybe:

The variant kind VK_PPC_NOTOC needs to be handled as a special case because we do not want the assembly to print out the @notoc at the end like __tls_get_addr(x@tlsgd)@notoc. Instead we want it to look like __tls_get_addr@notoc(x@tlsgd).
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
484

nit:
Can you add an assert above to make sure that this MI needs to have at least 3 operands?

490

Can we just set whatever is in the else condition as the default for Kind and Opcode?
I think this may be more of a style thing but I like having defaults for variables since that way it is harder for future code to accidentally end up with a situation where someone adds an else if and forgets to define one of them.

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

nit:
Line too long?

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

I'm a little worried about this setup.
In the case where IsTLSGDPCREL == true we end up with InReg not being assigned a register.
However, in that case we still do this:

const Register OrigRegs[] = {OutReg, InReg, GPR3};
// Some more code here...
LIS->repairIntervalsInRange(&MBB, First, Last, OrigRegs);

Is this a safe thing to do?
Perhaps in that scenario it would be better to only have

const Register OrigRegs[] = {OutReg, GPR3};
llvm/test/CodeGen/PowerPC/pcrel-tls-general-dynamic.ll
8

nit:
Start using mcpu=pwr10 instead of future.

10

nit:
Add a little description of the test here.

llvm/test/MC/PowerPC/pcrel-tls-general-dynamic-address-load-reloc.s
23

nit:
Try to avoid using C++ mangled names. They are harder to read.
I would say rename this function and the function in the test below to describe what the function does.
eg. GDAddrLoad or GDGetAddr

kamaub updated this revision to Diff 275727.Jul 6 2020, 8:46 AM
  • Rebasing branch to more recent version of master.
  • Adding asserts to ensure values are always what they are expected to be.
  • Code formating and comments added.
  • Conditional logic for register operands rearranged to avoid potential bugs in PPCTLSDynamicCall.cpp.
kamaub updated this revision to Diff 275731.Jul 6 2020, 8:59 AM
kamaub marked 7 inline comments as done.
  • Renaming test functions names as per review comments.
kamaub marked an inline comment as done.Jul 6 2020, 8:59 AM
stefanp accepted this revision as: stefanp.Jul 16 2020, 2:45 PM

I think the tests can be cleaned up a little but otherwise this LGTM.

llvm/test/MC/PowerPC/pcrel-tls-general-dynamic-address-load-reloc.s
41

This is something I've learned relatively recently but you can get rid of a lot of the "extras" that compiling from C code produces. For example you can reduce most of that assembly to:

GeneralDynamicAddrLoad:
	mflr 0
	std 0, 16(1)
	stdu 1, -32(1)
	paddi 3, 0, x@got@tlsgd@pcrel, 1
	bl __tls_get_addr@notoc(x@tlsgd)
	addi 1, 1, 32
	ld 0, 16(1)
	mtlr 0
	blr

And if you want to just look at the TLS relocation (which is what you are testing) you can even remove the stack setup and teardown and have the whole test be just:

# READOBJ:        0x0 R_PPC64_GOT_TLSGD_PCREL34 x 0x0
# READOBJ-NEXT:   0x8 R_PPC64_TLSGD x 0x0
# READOBJ-NEXT:   0x8 R_PPC64_REL24_NOTOC __tls_get_addr 0x0

GeneralDynamicAddrLoad:
	paddi 3, 0, x@got@tlsgd@pcrel, 1
	bl __tls_get_addr@notoc(x@tlsgd)
	blr
llvm/test/MC/PowerPC/pcrel-tls-general-dynamic-value-load-reloc.s
25

Same thing for this test. You can remove a lot of the extra lines and just keep what you are testing.

This revision is now accepted and ready to land.Jul 16 2020, 2:45 PM
kamaub updated this revision to Diff 283208.Aug 5 2020, 5:40 AM

Addressing review comments and updating test cases:

  • rebasing the patch on to a more recent master and fixed merge conflicts
  • changed test cases to remove unnecessary bloat and update run lines.
kamaub marked 2 inline comments as done.Aug 5 2020, 10:19 AM
kamaub updated this revision to Diff 283697.Aug 6 2020, 12:09 PM

Rebasing patch on D85448 and editing test cases to use enablement option.

This revision was landed with ongoing or failed builds.Aug 20 2020, 1:08 PM
This revision was automatically updated to reflect the committed changes.