GlobalsModRef, ValueTracking: Look through threadlocal.address intrinsic

[MatzeB]

This improves handling of threadlocal.address intrinsic in analyses:

The thread-id cannot change within a function with the exception of suspend points of pre-split coroutines. This changes llvm::getUnderlyingObject to look through threadlocal.address in these cases.

GlobalsAAResult::AnalyzeUsesOfPointer checks whether an address can be traced to simple loads/stores or escapes to other places. Starting the analysis from a thread-local GlobalValue the threadlocal.address intrinsic is safe to skip here.

This improves issue #87437

[llvmbot]

@llvm/pr-subscribers-backend-powerpc
@llvm/pr-subscribers-backend-x86

@llvm/pr-subscribers-llvm-analysis

Author: Matthias Braun (MatzeB)

Changes

Analyses should typically look through threadlocal.address intrinsic to get at the underlying GlobalValue. This fixes GlobalsAAResult::AnalyzeUsesOfPointer and llvm::getUnderlyingObject.

---

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

3 Files Affected:

• (modified) llvm/lib/Analysis/GlobalsModRef.cpp (+8)
• (modified) llvm/lib/Analysis/ValueTracking.cpp (+6)
• (modified) llvm/test/Analysis/GlobalsModRef/nonescaping-noalias.ll (+19)

diff --git a/llvm/lib/Analysis/GlobalsModRef.cpp b/llvm/lib/Analysis/GlobalsModRef.cpp
index 527f19b194eeb9..1ceb1b26294183 100644
--- a/llvm/lib/Analysis/GlobalsModRef.cpp
+++ b/llvm/lib/Analysis/GlobalsModRef.cpp
@@ -344,6 +344,14 @@ bool GlobalsAAResult::AnalyzeUsesOfPointer(Value *V,
       if (AnalyzeUsesOfPointer(I, Readers, Writers, OkayStoreDest))
         return true;
     } else if (auto *Call = dyn_cast<CallBase>(I)) {
+      if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(I)) {
+        if (II->getIntrinsicID() == Intrinsic::threadlocal_address &&
+            V == II->getArgOperand(0)) {
+          if (AnalyzeUsesOfPointer(II, Readers, Writers))
+            return true;
+          continue;
+        }
+      }
       // Make sure that this is just the function being called, not that it is
       // passing into the function.
       if (Call->isDataOperand(&U)) {
diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index 3a10de72a27562..d25ebd80d2411d 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -6269,6 +6269,12 @@ const Value *llvm::getUnderlyingObject(const Value *V, unsigned MaxLookup) {
           continue;
         }
       } else if (auto *Call = dyn_cast<CallBase>(V)) {
+        if (const IntrinsicInst *II = dyn_cast<IntrinsicInst>(V)) {
+          if (II->getIntrinsicID() == Intrinsic::threadlocal_address) {
+            V = II->getArgOperand(0);
+            continue;
+          }
+        }
         // CaptureTracking can know about special capturing properties of some
         // intrinsics like launder.invariant.group, that can't be expressed with
         // the attributes, but have properties like returning aliasing pointer.
diff --git a/llvm/test/Analysis/GlobalsModRef/nonescaping-noalias.ll b/llvm/test/Analysis/GlobalsModRef/nonescaping-noalias.ll
index d109b3b8748ba7..f28e73c48b3e8e 100644
--- a/llvm/test/Analysis/GlobalsModRef/nonescaping-noalias.ll
+++ b/llvm/test/Analysis/GlobalsModRef/nonescaping-noalias.ll
@@ -22,6 +22,25 @@ entry:
   ret i32 %v
 }
 
+@g1_tls = internal thread_local global i32 0
+
+define i32 @test1_tls(ptr %param) {
+; Ensure that we can fold a store to a load of a global across a store to
+; a parameter when the global is non-escaping.
+;
+; CHECK-LABEL: @test1_tls(
+; CHECK: %p = call ptr @llvm.threadlocal.address.p0(ptr @g1_tls)
+; CHECK: store i32 42, ptr %p
+; CHECK-NOT: load i32
+; CHECK: ret i32 42
+entry:
+  %p = call ptr @llvm.threadlocal.address(ptr @g1_tls)
+  store i32 42, ptr %p
+  store i32 7, ptr %param
+  %v = load i32, ptr %p
+  ret i32 %v
+}
+
 declare ptr @f()
 
 define i32 @test2() {

[jyknight]

I'm a bit worried of this, though I'm not sure if my worry is valid or not.

I worry that adding such code before we fix the underlying representation (to make the thread_local GlovalValue no longer incorrect pretend like it represents an address at the IR level) will end up making it more difficult to do so.

[MatzeB]

It's possible I missed most of the discussion around this intrinsic. To me this mostly felt like coroutines were breaking because the underlying thread could change across yield points?

At least for GlobalsAAResult::AnalyzeUsesOfPointer it shouldn't really matter at what thread we are looking at... I guess for llvm::getUnderlyingObject it could depend on what calling code is using it for whether changing threads are a problem. For the BasicAA usage it should be fine AFAIK, but admittedly can't necessary say it for other users...

Either way for my use cases here the intrinsic is showing up as a code quality regression compared to older LLVM versions...

[MatzeB]

At least if we sprinkle in more checks for llvm.threadlocal.address you have a good way to search for places that need updating if you want to change the representation later...

[MatzeB]

Added unittests for getUnderlyingObject in #89120 so I can add to them here.

[MatzeB]

Any way to move this forward?

There is no problem for correctness here:

• The code hasn't changed substantially since the llvm.threadlocal.address intrinsic was introduced and worked fine before. So skipping the intrinsic in getUnderlyingObject should have no meaningful difference to previous state.
• We need some representative entity for alias analysis that expresses easily that TLS vars are separate from other global entities and each other. The underlying GlobalVariable works well for that (while the @llvm.threadlocal.address intrinsic does not as it may be duplicated in multiple functions or even within the same function).

[MatzeB]

Found https://discourse.llvm.org/t/address-thread-identification-problems-with-coroutine/62015 and seeing now why this patch is a problem before coroutine splitting... Though still need to catch up on whether we actually developed any further mechanisms in the meantime to maybe express/mark coroutines before/after splitting.

[MatzeB]

Changed the new code to no longer skip threadlocal.address when isPresplitCoroutine() == true which should address the coroutine concerns (and as far as I can tell we currently have no other situations/attributes indicating that a function could switch thread-ids).

Adding some more reviewers given the discussion from https://discourse.llvm.org/t/address-thread-identification-problems-with-coroutine/62015

[MatzeB]

Some backend tests changed apparently because the improved getUnderlyingObject results in better alias analysis (and consequently different scheduling decisions in PowerPC).

[efriedma-quic]

LGTM

Making ValueTracking look though the intirnisc should be fine, I think? Certain IPO passes might need to be a little careful with how they deal with an identified thread-local, but that was true even before threadlocal.address was a thing.

GlobalsModRef seems fine; it never returns MustAlias, so approximating the thread-locals of all threads as a single "global" should be fine.

[nikic]

I think this is okay, but please replace those unit tests with lit tests, e.g. in llvm/test/Analysis/BasicAA. Test the actually desired effect on AA you want from this.

[chenzheng1030]

Didn't find obvious issue for the PowerPC code gen diffs.

FYI @amy-kwan @orcguru

[amy-kwan]

Agreed.

[MatzeB]

Test the actually desired effect on AA you want from this.

Dropping unittests, there were already IR tests added in llvm/test/Analysis/GlobalsModRef/nonescaping-noalias.ll

[MatzeB]

Seems the newly added test broke main (thanks Michael Liao for cleaning it up). I wonder why that didn't show in buildkite, doesn't seem to be caused by a recent change...