[Attributor] Don't replace addrspacecast (ptr null to ptr addrspace(x)) with ptr addrspace(x) null

[shiltian]

ConstantPointerNull represents a pointer with value 0, but it doesn’t
necessarily mean a nullptr. ptr addrspace(x) null is not the same as
addrspacecast (ptr null to ptr addrspace(x)) if the nullptr in AS X is not
zero. Therefore, we can't simply replace it.

Fixes #115083.

[llvmbot]

@llvm/pr-subscribers-backend-amdgpu

@llvm/pr-subscribers-llvm-transforms

Author: Shilei Tian (shiltian)

Changes

ConstantPointerNull represents a pointer with value 0, but it doesn’t
necessarily mean a nullptr. ptr addrspace(x) null is not the same as
addrspacecast (ptr null to ptr addrspace(x)) if the nullptr in AS x is not
zero. Therefore, we can't simply replace it.

Fixes #115083.

---

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

2 Files Affected:

• (modified) llvm/lib/Transforms/IPO/AttributorAttributes.cpp (+9)
• (added) llvm/test/Transforms/Attributor/do-not-replace-addrspacecast-with-constantpointernull.ll (+39)

diff --git a/llvm/lib/Transforms/IPO/AttributorAttributes.cpp b/llvm/lib/Transforms/IPO/AttributorAttributes.cpp
index 17e7fada1082762..143215ec91706b3 100644
--- a/llvm/lib/Transforms/IPO/AttributorAttributes.cpp
+++ b/llvm/lib/Transforms/IPO/AttributorAttributes.cpp
@@ -10970,6 +10970,15 @@ struct AAPotentialValuesImpl : AAPotentialValues {
       Value *NewV = getSingleValue(A, *this, getIRPosition(), Values);
       if (!NewV || NewV == &OldV)
         continue;
+      // FIXME: ConstantPointerNull represents a pointer with value 0, but it
+      // doesn’t necessarily mean a nullptr. `ptr addrspace(x) null` is not the
+      // same as `addrspacecast (ptr null to ptr addrspace(x))` if the nullptr
+      // in AS x is not zero. Therefore, we can't simply replace it.
+      if (isa<ConstantPointerNull>(NewV) && isa<ConstantExpr>(OldV)) {
+        auto &CE = cast<ConstantExpr>(OldV);
+        if (CE.getOpcode() == Instruction::AddrSpaceCast)
+          continue;
+      }
       if (getCtxI() &&
           !AA::isValidAtPosition({*NewV, *getCtxI()}, A.getInfoCache()))
         continue;
diff --git a/llvm/test/Transforms/Attributor/do-not-replace-addrspacecast-with-constantpointernull.ll b/llvm/test/Transforms/Attributor/do-not-replace-addrspacecast-with-constantpointernull.ll
new file mode 100644
index 000000000000000..77eb5fed81d3cfa
--- /dev/null
+++ b/llvm/test/Transforms/Attributor/do-not-replace-addrspacecast-with-constantpointernull.ll
@@ -0,0 +1,39 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt -S -passes=attributor $s -o - | FileCheck %s
+
+define i32 @bar(ptr %p0, ptr addrspace(5) %p5) {
+; CHECK-LABEL: define i32 @bar(
+; CHECK-SAME: ptr nofree readonly captures(none) [[P0:%.*]], ptr addrspace(5) nofree readonly [[P5:%.*]]) #[[ATTR0:[0-9]+]] {
+; CHECK-NEXT:  [[ENTRY:.*:]]
+; CHECK-NEXT:    [[TMP0:%.*]] = icmp eq ptr addrspace(5) [[P5]], addrspacecast (ptr null to ptr addrspace(5))
+; CHECK-NEXT:    [[TMP1:%.*]] = select i1 [[TMP0]], ptr [[P0]], ptr null
+; CHECK-NEXT:    switch i8 0, label %[[BB_2:.*]] [
+; CHECK-NEXT:      i8 0, label %[[BB_0:.*]]
+; CHECK-NEXT:      i8 1, label %[[BB_1:.*]]
+; CHECK-NEXT:    ]
+; CHECK:       [[BB_0]]:
+; CHECK-NEXT:    [[TMP2:%.*]] = load i32, ptr [[TMP1]], align 4
+; CHECK-NEXT:    ret i32 [[TMP2]]
+; CHECK:       [[BB_1]]:
+; CHECK-NEXT:    unreachable
+; CHECK:       [[BB_2]]:
+; CHECK-NEXT:    unreachable
+;
+entry:
+  %0 = icmp eq ptr addrspace(5) %p5, addrspacecast (ptr null to ptr addrspace(5))
+  %1 = select i1 %0, ptr %p0, ptr null
+  switch i8 0, label %bb.2 [
+  i8 0, label %bb.0
+  i8 1, label %bb.1
+  ]
+
+bb.0:                                             ; preds = %entry
+  %2 = load i32, ptr %1, align 4
+  ret i32 %2
+
+bb.1:                                             ; preds = %entry
+  ret i32 0
+
+bb.2:                                             ; preds = %entry
+  ret i32 1
+}

[shiltian]

• [Attributor] Don't replace addrspacecast (ptr null to ptr addrspace(x)) with ptr addrspace(x) null #126779 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[shiltian]

Ping.

AFAICT, only ConstantPointerNull matters here because it represents null in all AS. For the others, they are not constant thus will not be replaced by this code path.

[shiltian]

This will be properly resolved after #131557 (as well as its follow-up patches).

[shiltian]

I figured it's gonna take much longer time to change the semantics of ConstantPointerNull as well as all related stuff, so I pushed an alternative solution here.

[jdoerfert]

I'm not super convinced but it seems necessary for now.
LG