[Attributor] Don't replace AddrSpaceCast with ConstantPointerNull
#126779
Conversation
|
@llvm/pr-subscribers-llvm-transforms Author: Shilei Tian (shiltian) Changes
Fixes #115083. Full diff: https://github.com/llvm/llvm-project/pull/126779.diff 2 Files Affected:
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
+}
|
llvm/test/Transforms/Attributor/do-not-replace-addrspacecast-with-constantpointernull.ll
Outdated
Show resolved
Hide resolved
llvm/test/Transforms/Attributor/do-not-replace-addrspacecast-with-constantpointernull.ll
Outdated
Show resolved
Hide resolved
| // 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)) { |
There was a problem hiding this comment.
Does this handle nested subexpressions correctly?
There was a problem hiding this comment.
The result type must be a pointer so that leaves us to IntToPtr and AddrSpaceCast for the outermost expression. If it is IntToPtr, I think they can be directly replaced because the value will have to be 0. With that being said, I think this would work fine.
There was a problem hiding this comment.
It doesn't feel right to filter this during the manifest. I'd expect some other code to avoid looking through the cast, and then return the original expression such that the NewV == OldV check would naturally fail
There was a problem hiding this comment.
Specifically there are some stripPointerCasts that should be restricted to stripPointerCastsSameRepresentation
There was a problem hiding this comment.
Interestingly, that was exactly the first version of my fix, but I chose the current approach after a second thought.
We still want to look through address space cast when analyzing potential pointer value and/or underlying object. The only thing concerned here is probably nullptr. I think it is okay to treat ptr addrspace(x) null same as addrspacecast (ptr null to ptr addrspace(x)) when we do analysis such that we can know whether a pointer points to nothing, but we will not replace the latter with the former because of the limitation/issue/design we currently have.
On a side note, IMHO, they should be the same and ptr addrspace(x) null should represent the nullptr in AS x but the source code representation ConstantPointerNull means something else. Once we have a new class for DL nullptr, I think ptr addrspace(x) null should belong to it.
There was a problem hiding this comment.
We still want to look through address space cast when analyzing potential pointer value and/or underlying object.
It's okay to do this for underlying objects, because the semantics allow it. But I don't think it's valid for PotentialValues.
There was a problem hiding this comment.
Underlying object works on top of potential values, but it doesn't directly rely on the literal value of a pointer. My understanding is, whether the value would be the same after address space cast would not make any difference, as long as we don't replace the value directly, which is what this PR is trying to fix.
shiltian
force-pushed
the
users/shiltian/do-not-replace-addrspacecast-with-constantpointernull
branch
from
6ae31b3 to
23d8876
Compare
shiltian
force-pushed
the
users/shiltian/do-not-replace-addrspacecast-with-constantpointernull
branch
from
23d8876 to
dcd5a83
Compare
`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.
shiltian
force-pushed
the
users/shiltian/do-not-replace-addrspacecast-with-constantpointernull
branch
from
dcd5a83 to
93c20b2
Compare
|
Ping. AFAICT, only |
| if (isa<ConstantPointerNull>(NewV)) { | ||
| if (auto *CE = dyn_cast<ConstantExpr>(&OldV); | ||
| CE && CE->getOpcode() == Instruction::AddrSpaceCast) | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Checking this here, in manifest, and for this specific expression doesn't feel like the right way to address this.
I would trust it slightly more if it was a check on the address space of the pointer type, but I would still expect this logic to be somewhere else
There was a problem hiding this comment.
Can this sink somewhere down in getSingleValue?
| %select = select i1 %icmp, ptr %p0, ptr null | ||
| %load = load i32, ptr %select, align 4 | ||
| ret i32 %load | ||
| } |
There was a problem hiding this comment.
Also add another test with a differently typed null
|
This will be properly resolved after #131557 (as well as its follow-up patches). |
ConstantPointerNullrepresents a pointer with value 0, but it doesn’tnecessarily mean a nullptr.
ptr addrspace(x) nullis not the same asaddrspacecast (ptr null to ptr addrspace(x))if the nullptr in AS x is notzero. Therefore, we can't simply replace it.
Fixes #115083.