Page MenuHomePhabricator

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

Authored by Conanap on Jul 8 2020, 9:20 AM.

Details

Summary

This patch is the initial support for the Local Exec Thread 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.Jul 8 2020, 9:20 AM
kamaub added a reviewer: Restricted Project.Jul 8 2020, 9:26 AM
kamaub added a project: Restricted Project.
amyk added a subscriber: amyk.Jul 17 2020, 2:46 PM
amyk added inline comments.
llvm/lib/Target/PowerPC/PPCISelLowering.h
452

I think this might make more sense:

/// TLS_LOCAL_EXEC_MAT_ADDR = Materialize an address for TLS global address
/// when using local exec access models, and when prefixed instructions are
/// available. This is used with ADD_TLS to produce an add like PADDI.
llvm/test/CodeGen/PowerPC/pcrel-tls-local-exec.ll
9

nit: for the Local Exec TLS Model.

llvm/test/MC/PowerPC/pcrel-tls-local-exec-address-load-reloc.s
7

nit: with the address loaded.

NeHuang added inline comments.Jul 21 2020, 3:43 PM
llvm/test/MC/PowerPC/pcrel-tls-local-exec-address-load-reloc.s
6

nit x@TPREL

19

Please only keep the necessary assembly in the test and rename the function accordingly.

llvm/test/MC/PowerPC/pcrel-tls-local-exec-value-load-reloc.s
6

ditto

19

ditto

kamaub updated this revision to Diff 283377.Aug 5 2020, 1:42 PM

Rebasing on most recent D81947 and addressing review comments:

  • rebasing on the updated version of D81947
  • updating test cases according to review comments
  • updating comments in code
  • updating llvm-readobj run lines to follow new interface
kamaub marked 7 inline comments as done.Aug 5 2020, 1:43 PM
stefanp accepted this revision as: stefanp.Aug 20 2020, 2:27 PM

Only minor nits form me.
LGTM.

llvm/test/CodeGen/PowerPC/pcrel-tls-local-exec.ll
6

I think you are missing -enable-ppc-pcrel-tls here and in the command above.

llvm/test/MC/PowerPC/pcrel-tls-local-exec-address-load-reloc.s
14

nit:
You can remove the comment: # @LocalExec

llvm/test/MC/PowerPC/pcrel-tls-local-exec-value-load-reloc.s
14

nit:
Similarly I think you can remove the comment # @LocalExecLoad.

This revision is now accepted and ready to land.Aug 20 2020, 2:27 PM
MaskRay added inline comments.
llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFObjectWriter.cpp
424

Is this unreachable? If yes, assert may be better

llvm/test/CodeGen/PowerPC/pcrel-tls-local-exec.ll
7

Consider --no-show-raw-insn

stefanp requested changes to this revision.Aug 21 2020, 5:55 AM

Also, please add a test where the offset is not zero.
So the assembly would be for example:

paddi 3, 13, x@TPREL+8, 0

Sorry to have approved it and then requested changes. :(

llvm/test/CodeGen/PowerPC/pcrel-tls-local-exec.ll
31

I just realized this issue this morning.
We can probably do better here.
Instead of:

paddi r3, r13, x@TPREL, 0
lwz r3, <OFFSET>(r3)

we can probably generate:

plwz r3, x@TPREL+<OFFSET>(r13)

Where <OFFSET> is just some value.

This revision now requires changes to proceed.Aug 21 2020, 5:55 AM
stefanp added inline comments.Aug 24 2020, 12:09 PM
llvm/test/CodeGen/PowerPC/pcrel-tls-local-exec.ll
31

After an internal discussion it has been decided that we can leave this improvement for another patch. At this point I would say that we should just extend the test case to include offsets but not implement the merging of those two instructions.

kamaub added inline comments.Sep 1 2020, 12:00 PM
llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFObjectWriter.cpp
424

This switch was originally commit in rGbd2068031121adf5a0e28d9306a1741d6f0bbd87 but was reverted by rGce1e4853b5a15d679bd662ac5777a2390daf0391 because of failures in Release+Asserts mode with an assert. So rG97470897c436a6a5d682fb8ab296d0bcdc6e32a4 committed it as an report_fatal_error() to resolve the problem. I am not sure that assert() won't cause a similar problem as llvm_unreable() since we replaced llvm_unreachable() with report_fatal_error() for similar reason to why they suggest using assert() with llvm_unreachable() in

https://llvm.org/docs/CodingStandards.html#assert-liberally:
This has a few issues, the main one being that some compilers might not understand the assertion, or warn about a missing return in builds where assertions are compiled out.

Today, we have something much better: llvm_unreachable:

I could be wrong though, should we still replace it with an assert? I think the answer to your question is yes, it was reachable.

Conanap commandeered this revision.Sep 2 2020, 12:42 PM
Conanap added a reviewer: kamaub.
Conanap added a subscriber: Conanap.

Taking over for the patch

Conanap updated this revision to Diff 289544.Sep 2 2020, 12:46 PM
Conanap marked 7 inline comments as done.

Added test case for non-zero offset, added a few flags to the RUN lines and removed some unecessary comments.

Conanap updated this revision to Diff 289861.Sep 3 2020, 9:43 PM

Added an extra test case

Conanap updated this revision to Diff 289864.Sep 3 2020, 9:44 PM

Added back the test files that were accidentally removed in prev diff

stefanp accepted this revision.Sep 8 2020, 12:08 PM

This mostly LGTM.
From my perspective you can add no-show-raw-insn on commit.

llvm/test/CodeGen/PowerPC/pcrel-tls-local-exec.ll
7

I agree. Please use no-show-raw-insn!

66

Thank you for adding this test.
I think we should be able to do better here (as we can merge the offset from the add into the paddi) but I think that should be a separate patch.

This revision is now accepted and ready to land.Sep 8 2020, 12:08 PM
Conanap updated this revision to Diff 291350.Sep 11 2020, 3:18 PM
Conanap marked 2 inline comments as done.

Added --no-show-raw-insn and updated test case accordingly

This revision was automatically updated to reflect the committed changes.