This is an archive of the discontinued LLVM Phabricator instance.

Use @llvm.threadlocal.address intrinsic to access TLS
ClosedPublic

Authored by ChuanqiXu on Jul 14 2022, 10:41 PM.

Details

Summary

This is split for easier reading. The clang part of D125291. This revision would try to use @llvm.threadlocal.address in clang to access TLS variable. The reason why the OpenMP tests contains a lot of change is that they uses utils/update_cc_test_checks.py to update their tests.

Diff Detail

Event Timeline

ChuanqiXu created this revision.Jul 14 2022, 10:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2022, 10:41 PM
ChuanqiXu requested review of this revision.Jul 14 2022, 10:41 PM
nikic accepted this revision.Jul 29 2022, 6:18 AM

LG. I'm not sure this covers all places where threadlocal.address should get emitted, but I think we also don't need to get this correct right away -- for now, we we'll be able to observe if this has any negative impact on optimization, and any remaining codes will be found when we switch to a token type.

clang/lib/CodeGen/CGExpr.cpp
2887

Can use withPointer here as well?

This revision is now accepted and ready to land.Jul 29 2022, 6:18 AM
ChuanqiXu updated this revision to Diff 448906.Jul 31 2022, 8:02 PM

Address comments.

ChuanqiXu marked an inline comment as done.Jul 31 2022, 8:03 PM

LG. I'm not sure this covers all places where threadlocal.address should get emitted, but I think we also don't need to get this correct right away -- for now, we we'll be able to observe if this has any negative impact on optimization, and any remaining codes will be found when we switch to a token type.

Agreed. Thanks for reviewing!

This revision was landed with ongoing or failed builds.Jul 31 2022, 8:05 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 31 2022, 8:05 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Note that this change caused LLVM to no longer be aware that a TLS variable cannot be NULL. Thus, code like:

__thread int x;
int main() {
  int* y = &x;
  return *y;
}

built with clang -O -fsanitize=null emits a null-pointer check, when it wouldn't previously. I think llvm.threadlocal.address's return value probably ought to be given the nonnull attribute. We also might want to infer other properties of the returned pointer based on knowledge of the global value parameter, such as alignment.

Furthermore, while the above problem should simply be a very minor optimization degradation, in fact it caused miscompiles, due to a pre-existing bug in the X86 backend. I've sent https://reviews.llvm.org/D131716 to fix that part of the problem.

Note that this change caused LLVM to no longer be aware that a TLS variable cannot be NULL. Thus, code like:

__thread int x;
int main() {
  int* y = &x;
  return *y;
}

built with clang -O -fsanitize=null emits a null-pointer check, when it wouldn't previously. I think llvm.threadlocal.address's return value probably ought to be given the nonnull attribute. We also might want to infer other properties of the returned pointer based on knowledge of the global value parameter, such as alignment.

Furthermore, while the above problem should simply be a very minor optimization degradation, in fact it caused miscompiles, due to a pre-existing bug in the X86 backend. I've sent https://reviews.llvm.org/D131716 to fix that part of the problem.

Oh, got it. I'll try to make it. And I find we can't set an intrinsic as NonNull directly in Intrinsics.td and I filed an issue for it: https://github.com/llvm/llvm-project/issues/57113.

Note that this change caused LLVM to no longer be aware that a TLS variable cannot be NULL. Thus, code like:

__thread int x;
int main() {
  int* y = &x;
  return *y;
}

built with clang -O -fsanitize=null emits a null-pointer check, when it wouldn't previously. I think llvm.threadlocal.address's return value probably ought to be given the nonnull attribute. We also might want to infer other properties of the returned pointer based on knowledge of the global value parameter, such as alignment.

Furthermore, while the above problem should simply be a very minor optimization degradation, in fact it caused miscompiles, due to a pre-existing bug in the X86 backend. I've sent https://reviews.llvm.org/D131716 to fix that part of the problem.

The NonNull attribute is added in https://github.com/llvm/llvm-project/commit/d68ba43ad24791181280fdb0f34b6be380db7a32.

And I am working on adding Align properties. But I meet problems since the alignment of threadlocal_address intrinsic depends on its argument so we can't set the alignment for its declaration and we probably need to set the alignment for its call, which means we need to set the alignment when in IRBuilder. Do you think this is good?

nikic added a comment.Aug 29 2022, 4:04 AM

And I am working on adding Align properties. But I meet problems since the alignment of threadlocal_address intrinsic depends on its argument so we can't set the alignment for its declaration and we probably need to set the alignment for its call, which means we need to set the alignment when in IRBuilder. Do you think this is good?

I think that would be fine. Alternatively, it could be inferred in InstCombine.

And I am working on adding Align properties. But I meet problems since the alignment of threadlocal_address intrinsic depends on its argument so we can't set the alignment for its declaration and we probably need to set the alignment for its call, which means we need to set the alignment when in IRBuilder. Do you think this is good?

I think that would be fine. Alternatively, it could be inferred in InstCombine.

Thanks. I've filed an issue for it: https://github.com/llvm/llvm-project/issues/57438. (I'll fix it later if I had time)