Page MenuHomePhabricator

[AArch64] Add -mtls-size option for ELF targets
ClosedPublic

Authored by kawashima-fj on Dec 18 2019, 10:23 PM.

Details

Summary

This option allows selecting the TLS size in the local exec TLS model,
which is the default TLS model for non-PIC objects. This allows large/
many thread local variables or a compact/fast code in an executable.

Specification is same as that of GCC. For example, the code model
option precedes the TLS size option.

TLS access models other than local-exec are not changed. It means
supoort of the large code model is only in the local exec TLS model.

Diff Detail

Event Timeline

kawashima-fj created this revision.Dec 18 2019, 10:23 PM

Directory prefixes were deleted by git show --no-prefix in the previous diff. It is corrected now.

This is the same as gcc? Sounds good.

I'm adding some people who may remember more about AArch64 TLS codegen.

This is the same as gcc? Sounds good.

Yes. I confirmed its behaivior and output (not source code).

Apologies, I'm out of office for the next couple of weeks. If you don't get any other volunteers to review can you ping again in January, I'll be back on the 6th.

Apologies for the delay in responding, just come back from vacation. I've checked the implementation against GCC and it looks like it will give the same behaviour. I've got one minor suggestion surrounding the clamping of TLSSize to its maximum value. It looks like it would only need to be done once, is there a convenient place, such as AArch64TargetMachine where it can be done once?

Apologies for the delay in responding, just come back from vacation. I've checked the implementation against GCC and it looks like it will give the same behaviour. I've got one minor suggestion surrounding the clamping of TLSSize to its maximum value. It looks like it would only need to be done once, is there a convenient place, such as AArch64TargetMachine where it can be done once?

No need to apologize. I know many people are in vacation last two or three weeks.

I'm not sure whether I understand your suggestion correctly. Do you mean the following code in the LowerELFTLSLocalExec function should be in a function in AArch64TargetMachine or somewhere which is called only once because LowerELFTLSLocalExec is called for every thread local variable? If so, it makes sense. I'll find a convenient place.

if (TLSSize == 0) // default
  TLSSize = 24;
if ((CModel == CodeModel::Small || CModel == CodeModel::Kernel) &&
    TLSSize > 32)
  // for the small (and kernel) code model, the maximum TLS size is 4GiB
  TLSSize = 32;
else if (CModel == CodeModel::Tiny && TLSSize > 24)
  // for the tiny code model, the maximum TLS size is 1MiB (< 16MiB)
  TLSSize = 24;

Apologies for the delay in responding, just come back from vacation. I've checked the implementation against GCC and it looks like it will give the same behaviour. I've got one minor suggestion surrounding the clamping of TLSSize to its maximum value. It looks like it would only need to be done once, is there a convenient place, such as AArch64TargetMachine where it can be done once?

No need to apologize. I know many people are in vacation last two or three weeks.

I'm not sure whether I understand your suggestion correctly. Do you mean the following code in the LowerELFTLSLocalExec function should be in a function in AArch64TargetMachine or somewhere which is called only once because LowerELFTLSLocalExec is called for every thread local variable? If so, it makes sense. I'll find a convenient place.

if (TLSSize == 0) // default
  TLSSize = 24;
if ((CModel == CodeModel::Small || CModel == CodeModel::Kernel) &&
    TLSSize > 32)
  // for the small (and kernel) code model, the maximum TLS size is 4GiB
  TLSSize = 32;
else if (CModel == CodeModel::Tiny && TLSSize > 24)
  // for the tiny code model, the maximum TLS size is 1MiB (< 16MiB)
  TLSSize = 24;

Ideally somewhere only called once, I mentioned AArch64TargetMachine as it came to mind as a potential example although there may be better places available. Apologies for the confusion.

Thanks for a suggestion. I looked around the code and found that the suggested AArch64TargetMachine constructor is the best place because other option values are set there. The updated patch moved the maximum value setting code to that constructor.

peter.smith accepted this revision.Jan 8 2020, 1:18 AM

Thanks for the update, looks good to me.

This revision is now accepted and ready to land.Jan 8 2020, 1:18 AM

Thanks for review.

I don't have commit access. If there are no comments form other people, could someone commit this change?
This is my first patch to the LLVM community. If I forgot something, please let me know.
Thanks!

I can do it early next week, unfortunately a little too busy today. If anyone else can do it earlier please go ahead. To obtain commit access it is worth following https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

This revision was automatically updated to reflect the committed changes.

Committed as 10c11e4e2d05cf0e8f8251f50d84ce77eb1e9b8d , I've used the new attribution process https://llvm.org/docs/DeveloperPolicy.html so you should show up as the author of the patch in the commit log. I'll comment here if there are any problems with buildbots.