Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

InstCombine: Increase threadlocal.address alignment if pointee is more aligned #88435

Merged
merged 2 commits into from
Apr 16, 2024

Conversation

MatzeB
Copy link
Contributor

@MatzeB MatzeB commented Apr 11, 2024

Increase alignment of llvm.threadlocal.address if the pointed to global has higher alignment.

@MatzeB MatzeB marked this pull request as ready for review April 11, 2024 23:18
@MatzeB MatzeB requested a review from nikic as a code owner April 11, 2024 23:18
@llvmbot
Copy link
Collaborator

llvmbot commented Apr 11, 2024

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-llvm-analysis

Author: Matthias Braun (MatzeB)

Changes

Look through llvm.threadlocal.address intrinsic when computing known bits in ValueTracking.

This improves issue #87437


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

2 Files Affected:

  • (modified) llvm/lib/Analysis/ValueTracking.cpp (+4)
  • (added) llvm/test/Analysis/ValueTracking/knownbits-tls.ll (+32)
diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index 3a10de72a27562..c7aec8518044f3 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -1686,6 +1686,10 @@ static void computeKnownBitsFromOperator(const Operator *I,
           Known.Zero.setBitsFrom(KnownZeroFirstBit);
         break;
       }
+      case Intrinsic::threadlocal_address: {
+        computeKnownBits(II->getArgOperand(0), Known, Depth + 1, Q);
+        break;
+      }
       case Intrinsic::vscale: {
         if (!II->getParent() || !II->getFunction())
           break;
diff --git a/llvm/test/Analysis/ValueTracking/knownbits-tls.ll b/llvm/test/Analysis/ValueTracking/knownbits-tls.ll
new file mode 100644
index 00000000000000..203777c33f8486
--- /dev/null
+++ b/llvm/test/Analysis/ValueTracking/knownbits-tls.ll
@@ -0,0 +1,32 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 4
+; RUN: opt -S -passes=instcombine < %s | FileCheck %s
+
+target datalayout = "p:64:64"
+
+@tlsvar_align32 = thread_local global i32 3, align 32
+
+define i1 @lowbits_zero() {
+; CHECK-LABEL: define i1 @lowbits_zero() {
+; CHECK-NEXT:    ret i1 true
+;
+  %p = call ptr @llvm.threadlocal.address(ptr @tlsvar_align32)
+  %p_int = ptrtoint ptr %p to i64
+  %lowbits = and i64 %p_int, 31
+  %lowbits_zero = icmp eq i64 %lowbits, 0
+  ret i1 %lowbits_zero
+}
+
+define i1 @lowbits_unknown() {
+; CHECK-LABEL: define i1 @lowbits_unknown() {
+; CHECK-NEXT:    [[P:%.*]] = call ptr @llvm.threadlocal.address.p0(ptr @tlsvar_align32)
+; CHECK-NEXT:    [[P_INT:%.*]] = ptrtoint ptr [[P]] to i64
+; CHECK-NEXT:    [[LOWBITS:%.*]] = and i64 [[P_INT]], 32
+; CHECK-NEXT:    [[LOWBITS_ZERO:%.*]] = icmp eq i64 [[LOWBITS]], 0
+; CHECK-NEXT:    ret i1 [[LOWBITS_ZERO]]
+;
+  %p = call ptr @llvm.threadlocal.address(ptr @tlsvar_align32)
+  %p_int = ptrtoint ptr %p to i64
+  %lowbits = and i64 %p_int, 63
+  %lowbits_zero = icmp eq i64 %lowbits, 0
+  ret i1 %lowbits_zero
+}

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative, and I think maybe better, approach to this would be to infer an align return attribute on threadlocal.address in InstCombine. This way, not only will computeKnownBits() automatically handle it, but it will also benefit other APIs that directly look at alignment, such as getPointerAlign().

@MatzeB
Copy link
Contributor Author

MatzeB commented Apr 12, 2024

Yep, I also just noticed that IRBuilder::CreateThreadLocalAddress already transfers the alignment of the target which I missed when I manually created LLVM-IR for tests. Anyway having an InstCombine rule that transfers minimum alignment is a good idea as well. Changed.

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@MatzeB MatzeB changed the title ValueTracking: Skip threadlocal.address in computeKnownBits InstCombine: Increase threadlocal.address alignment if pointee is more aligned Apr 13, 2024
@MatzeB MatzeB merged commit d23a850 into llvm:main Apr 16, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants