[TargetLowering] Fix the problem of emulated-TLS implementation witho… #101490

cceerczw · 2024-08-01T14:21:52Z

For a __thread variable x, when emulated TLS is enabled and there is an access to x, the compiler first looks up the symbol __emutls_v.x within the module. However, the issue arises with an alias y of x, the compiler still tries to look up __emutls_v.y instead of __emutls_v.x. As a result, the lookup returns a nullptr, causing the compiler to crash. The purpose of this MR (Merge Request) is to ensure that in emulated TLS, before checking __emutls_v.y, the compiler first identifies which global value y is an alias of.

github-actions · 2024-08-01T14:22:44Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2024-08-01T14:23:20Z

@llvm/pr-subscribers-backend-aarch64

@llvm/pr-subscribers-llvm-selectiondag

Author: None (cceerczw)

Changes

For a __thread variable x, when emulated TLS is enabled and there is an access to x, the compiler first looks up the symbol __emutls_v.x within the module. However, the issue arises with an alias y of x, the compiler still tries to look up __emutls_v.y instead of __emutls_v.x. As a result, the lookup returns a nullptr, causing the compiler to crash. The purpose of this MR (Merge Request) is to ensure that in emulated TLS, before checking __emutls_v.y, the compiler first identifies which global value y is an alias of.

Full diff: https://github.com/llvm/llvm-project/pull/101490.diff

2 Files Affected:

(modified) llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp (+4-2)
(added) llvm/test/CodeGen/AArch64/emutls_alias.ll (+20)

diff --git a/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp b/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
index 6fd23b5ab9f5f..e61f05445030d 100644
--- a/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
@@ -10225,8 +10225,10 @@ SDValue TargetLowering::LowerToTLSEmulatedModel(const GlobalAddressSDNode *GA,
 
   ArgListTy Args;
   ArgListEntry Entry;
-  std::string NameString = ("__emutls_v." + GA->getGlobal()->getName()).str();
-  Module *VariableModule = const_cast<Module*>(GA->getGlobal()->getParent());
+  const GlobalValue *GV = GA->getGlobal();
+  GV = GV->getAliaseeObject() ? GV->getAliaseeObject() : GV;
+  std::string NameString = ("__emutls_v." + GV->getName()).str();
+  Module *VariableModule = const_cast<Module*>(GV->getParent());
   StringRef EmuTlsVarName(NameString);
   GlobalVariable *EmuTlsVar = VariableModule->getNamedGlobal(EmuTlsVarName);
   assert(EmuTlsVar && "Cannot find EmuTlsVar ");
diff --git a/llvm/test/CodeGen/AArch64/emutls_alias.ll b/llvm/test/CodeGen/AArch64/emutls_alias.ll
new file mode 100644
index 0000000000000..78a08fe454dd9
--- /dev/null
+++ b/llvm/test/CodeGen/AArch64/emutls_alias.ll
@@ -0,0 +1,20 @@
+; RUN: llc < %s -emulated-tls -mtriple=aarch64-linux-ohos -O0 \
+; RUN:     | FileCheck -check-prefix=EMUTLS_CHECK %s
+ 
+%struct.__res_state = type { [5 x i8] }
+ 
+@foo = dso_local thread_local global %struct.__res_state { [5 x i8] c"\01\02\03\04\05" }, align 1
+ 
+@bar = hidden thread_local(initialexec) alias %struct.__res_state, ptr @foo
+ 
+; Function Attrs: noinline nounwind optnone uwtable
+define dso_local i32 @main() #0 {
+  %1 = alloca i32, align 4
+  store i32 0, ptr %1, align 4
+  store i8 0, ptr @bar, align 1
+  ; EMUTLS_CHECK: adrp    x0, __emutls_v.foo
+  ; EMUTLS_CHECK-NEXT: add     x0, x0, :lo12:__emutls_v.foo
+  ret i32 0
+}
+ 
+attributes #0 = { noinline nounwind optnone uwtable "frame-pointer"="non-leaf" "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="generic" "target-features"="+neon,+v8a" }

arsenm · 2024-08-01T14:26:29Z

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

+  const GlobalValue *GV = GA->getGlobal();
+  GV = GV->getAliaseeObject() ? GV->getAliaseeObject() : GV;


use stripPointerCastsAndAliases

arsenm · 2024-08-01T14:26:41Z

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

-  Module *VariableModule = const_cast<Module*>(GA->getGlobal()->getParent());
+  const GlobalValue *GV = GA->getGlobal();
+  GV = GV->getAliaseeObject() ? GV->getAliaseeObject() : GV;
+  std::string NameString = ("__emutls_v." + GV->getName()).str();


SmallString

The original code used 'NameString'. And the lenth of rvalue at line 10230 is indeterminate. So I don't think it's a good idea to use 'SmallString'.

It works for any size of string, it's just optimized for the common case where the name is small

arsenm · 2024-08-01T14:27:09Z

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

+  const GlobalValue *GV = GA->getGlobal();
+  GV = GV->getAliaseeObject() ? GV->getAliaseeObject() : GV;
+  std::string NameString = ("__emutls_v." + GV->getName()).str();
+  Module *VariableModule = const_cast<Module*>(GV->getParent());


Shouldn't need const_cast

If the 'const_cast' was removed, there will be an error caused:

Yes, you should also make VariableModule const

arsenm · 2024-08-01T14:27:42Z

llvm/test/CodeGen/AArch64/emutls_alias.ll

+  ret i32 0
+}
+
+attributes #0 = { noinline nounwind optnone uwtable "frame-pointer"="non-leaf" "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="generic" "target-features"="+neon,+v8a" }


Don't need all these attributes

arsenm · 2024-08-01T14:27:53Z

llvm/test/CodeGen/AArch64/emutls_alias.ll

+
+@bar = hidden thread_local(initialexec) alias %struct.__res_state, ptr @foo
+
+; Function Attrs: noinline nounwind optnone uwtable


Remove comment

arsenm · 2024-08-02T07:56:10Z

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

+  GV = GV->stripPointerCastsAndAliases()
+	   ? cast<GlobalValue>(GV->stripPointerCastsAndAliases()) : GV;


GV = GA->getGlobal()->stripPointerCastsAndAliases(), nothing is conditional

cceerczw · 2024-08-07T02:58:31Z

hi, @arsenm, thanks for the review, has any other reviews?

arsenm · 2024-08-07T06:09:52Z

llvm/test/CodeGen/AArch64/emutls_alias.ll

@@ -0,0 +1,17 @@
+; RUN: llc < %s -emulated-tls -mtriple=aarch64-linux-ohos -O0 \


Don't need -O0

arsenm · 2024-08-07T06:13:04Z

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

-  std::string NameString = ("__emutls_v." + GA->getGlobal()->getName()).str();
-  Module *VariableModule = const_cast<Module*>(GA->getGlobal()->getParent());
+  const GlobalValue *GV = cast<GlobalValue>(GA->getGlobal()->stripPointerCastsAndAliases());
+  SmallString<32> NameString = StringRef("__emutls_v." + GV->getName().str());


Suggested change

SmallString<32> NameString = StringRef("__emutls_v." + GV->getName().str());

SmallString<32> NameString("__emutls_v.");

NameString += GV->getName();

github-actions · 2024-08-07T13:19:12Z

✅ With the latest revision this PR passed the C/C++ code formatter.

…ut checking alias of current global value at first. For a __thread variable x, when emulated TLS is enabled and there is an access to x, the compiler first looks up the symbol __emutls_v.x within the module. However, the issue arises with an alias y of x, the compiler still tries to look up __emutls_v.y instead of __emutls_v.x. As a result, the lookup returns a nullptr, causing the compiler to crash. The purpose of this MR (Merge Request) is to ensure that in emulated TLS, before checking __emutls_v.y, the compiler first identifies which global value y is an alias of.

github-actions · 2024-08-07T17:57:09Z

@cceerczw Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested
by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as
the builds can include changes from many authors. It is not uncommon for your
change to be included in a build that fails due to someone else's changes, or
infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself.
This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

llvmbot added backend:AArch64 llvm:SelectionDAG SelectionDAGISel as well labels Aug 1, 2024

arsenm reviewed Aug 1, 2024

View reviewed changes

cceerczw force-pushed the fix-elumated-TLS branch from ea72068 to cd9d03a Compare August 2, 2024 07:31

arsenm reviewed Aug 2, 2024

View reviewed changes

cceerczw force-pushed the fix-elumated-TLS branch 2 times, most recently from 7d3771e to 8c55ead Compare August 5, 2024 08:09

arsenm reviewed Aug 7, 2024

View reviewed changes

cceerczw force-pushed the fix-elumated-TLS branch from 8c55ead to e60e723 Compare August 7, 2024 13:14

cceerczw force-pushed the fix-elumated-TLS branch from e60e723 to 75cb304 Compare August 7, 2024 13:23

arsenm approved these changes Aug 7, 2024

View reviewed changes

arsenm merged commit 6f8e8fa into llvm:main Aug 7, 2024
5 of 7 checks passed

		const GlobalValue *GV = GA->getGlobal();
		GV = GV->getAliaseeObject() ? GV->getAliaseeObject() : GV;


		@bar = hidden thread_local(initialexec) alias %struct.__res_state, ptr @foo

		; Function Attrs: noinline nounwind optnone uwtable

		GV = GV->stripPointerCastsAndAliases()
		? cast<GlobalValue>(GV->stripPointerCastsAndAliases()) : GV;

		@@ -0,0 +1,17 @@
		; RUN: llc < %s -emulated-tls -mtriple=aarch64-linux-ohos -O0 \

-  SmallString<32> NameString = StringRef("__emutls_v." + GV->getName().str());
+  SmallString<32> NameString("__emutls_v.");
+  NameString += GV->getName();

[TargetLowering] Fix the problem of emulated-TLS implementation witho… #101490

[TargetLowering] Fix the problem of emulated-TLS implementation witho… #101490

Uh oh!

Conversation

cceerczw commented Aug 1, 2024

Uh oh!

github-actions bot commented Aug 1, 2024

Uh oh!

llvmbot commented Aug 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cceerczw commented Aug 7, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Aug 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Aug 7, 2024

Uh oh!

Uh oh!

llvmbot commented Aug 1, 2024 •

edited

Loading

github-actions bot commented Aug 7, 2024 •

edited

Loading