[ValueTracking] Return true for AddrSpaceCast in canCreateUndefOrPoison #144686
Conversation
…invalid addrspacecast inst In our downstream GPU target, following IR is valid before instcombine although the second addrspacecast causes UB. define i1 @test(ptr addrspace(1) noundef %v) { %0 = addrspacecast ptr addrspace(1) %v to ptr addrspace(4) %1 = call i32 @llvm.xxxx.isaddr.shared(ptr addrspace(4) %0) %2 = icmp eq i32 %1, 0 %3 = addrspacecast ptr addrspace(4) %0 to ptr addrspace(3) %4 = select i1 %2, ptr addrspace(3) null, ptr addrspace(3) %3 %5 = icmp eq ptr addrspace(3) %4, null ret i1 %5 } We have a custom optimization that replaces invalid addrspacecast with poison, and IR is still valid since `select` stops poison propagation. However, instcombine pass optimizes `select` to `or`: %0 = addrspacecast ptr addrspace(1) %v to ptr addrspace(4) %1 = call i32 @llvm.xxxx.isaddr.shared(ptr addrspace(4) %0) %2 = icmp eq i32 %1, 0 %3 = addrspacecast ptr addrspace(1) %v to ptr addrspace(3) %4 = icmp eq ptr addrspace(3) %3, null %5 = or i1 %2, %4 ret i1 %5 The transform is invalid for our target.
llvmbot
added
backend:AMDGPU
llvm:instcombine
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-backend-amdgpu Author: Wenju He (wenju-he) ChangesIn our downstream GPU target, following IR is valid before instcombine although the second addrspacecast causes UB. However, instcombine pass optimizes Full diff: https://github.com/llvm/llvm-project/pull/144686.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
index 73ba0f78e8053..a2335640f917b 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
@@ -3194,8 +3194,23 @@ static Instruction *foldNestedSelects(SelectInst &OuterSelVal,
/// Return true if V is poison or \p Expected given that ValAssumedPoison is
/// already poison. For example, if ValAssumedPoison is `icmp samesign X, 10`
/// and V is `icmp ne X, 5`, impliesPoisonOrCond returns true.
-static bool impliesPoisonOrCond(const Value *ValAssumedPoison, const Value *V,
- bool Expected) {
+static bool impliesPoisonOrCond(
+ const Value *ValAssumedPoison, const Value *V, bool Expected,
+ llvm::function_ref<bool(unsigned, unsigned)> isValidAddrSpaceCast) {
+ // Handle the case that ValAssumedPoison is `icmp eq ptr addrspace(3) X, null`
+ // and X is `addrspacecast ptr addrspace(1) Y to ptr addrspace(3)`. Target can
+ // replace X with poison if the addrspacecast is invalid. However, `V` might
+ // not be poison.
+ if (auto *ICmp = dyn_cast<ICmpInst>(ValAssumedPoison)) {
+ auto CanCreatePoison = [&](Value *Op) {
+ auto *ASC = dyn_cast<AddrSpaceCastInst>(Op);
+ return ASC && !isValidAddrSpaceCast(ASC->getDestAddressSpace(),
+ ASC->getSrcAddressSpace());
+ };
+ if (llvm::any_of(ICmp->operands(), CanCreatePoison))
+ return false;
+ }
+
if (impliesPoison(ValAssumedPoison, V))
return true;
@@ -3241,17 +3256,23 @@ Instruction *InstCombinerImpl::foldSelectOfBools(SelectInst &SI) {
auto *Zero = ConstantInt::getFalse(SelType);
Value *A, *B, *C, *D;
+ auto IsValidAddrSpaceCast = [&](unsigned FromAS, unsigned ToAS) {
+ return isValidAddrSpaceCast(FromAS, ToAS);
+ };
+
// Folding select to and/or i1 isn't poison safe in general. impliesPoison
// checks whether folding it does not convert a well-defined value into
// poison.
if (match(TrueVal, m_One())) {
- if (impliesPoisonOrCond(FalseVal, CondVal, /*Expected=*/false)) {
+ if (impliesPoisonOrCond(FalseVal, CondVal, /*Expected=*/false,
+ IsValidAddrSpaceCast)) {
// Change: A = select B, true, C --> A = or B, C
return BinaryOperator::CreateOr(CondVal, FalseVal);
}
if (match(CondVal, m_OneUse(m_Select(m_Value(A), m_One(), m_Value(B)))) &&
- impliesPoisonOrCond(FalseVal, B, /*Expected=*/false)) {
+ impliesPoisonOrCond(FalseVal, B, /*Expected=*/false,
+ IsValidAddrSpaceCast)) {
// (A || B) || C --> A || (B | C)
return replaceInstUsesWith(
SI, Builder.CreateLogicalOr(A, Builder.CreateOr(B, FalseVal)));
@@ -3287,13 +3308,15 @@ Instruction *InstCombinerImpl::foldSelectOfBools(SelectInst &SI) {
}
if (match(FalseVal, m_Zero())) {
- if (impliesPoisonOrCond(TrueVal, CondVal, /*Expected=*/true)) {
+ if (impliesPoisonOrCond(TrueVal, CondVal, /*Expected=*/true,
+ IsValidAddrSpaceCast)) {
// Change: A = select B, C, false --> A = and B, C
return BinaryOperator::CreateAnd(CondVal, TrueVal);
}
if (match(CondVal, m_OneUse(m_Select(m_Value(A), m_Value(B), m_Zero()))) &&
- impliesPoisonOrCond(TrueVal, B, /*Expected=*/true)) {
+ impliesPoisonOrCond(TrueVal, B, /*Expected=*/true,
+ IsValidAddrSpaceCast)) {
// (A && B) && C --> A && (B & C)
return replaceInstUsesWith(
SI, Builder.CreateLogicalAnd(A, Builder.CreateAnd(B, TrueVal)));
diff --git a/llvm/test/Transforms/InstCombine/AMDGPU/addrspacecast.ll b/llvm/test/Transforms/InstCombine/AMDGPU/addrspacecast.ll
new file mode 100644
index 0000000000000..4791d2c434884
--- /dev/null
+++ b/llvm/test/Transforms/InstCombine/AMDGPU/addrspacecast.ll
@@ -0,0 +1,23 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt -S -mtriple=amdgcn-amd-amdhsa -passes=instcombine %s | FileCheck %s
+
+; Check that `select B, true, C` isn't optimized to `or B, C`.
+define i1 @not_fold_select(ptr addrspace(1) noundef %x) {
+; CHECK-LABEL: define i1 @not_fold_select(
+; CHECK-SAME: ptr addrspace(1) noundef [[X:%.*]]) {
+; CHECK-NEXT: [[ENTRY:.*:]]
+; CHECK-NEXT: [[TMP0:%.*]] = addrspacecast ptr addrspace(1) [[X]] to ptr
+; CHECK-NEXT: [[TMP1:%.*]] = tail call i1 @llvm.amdgcn.is.shared(ptr [[TMP0]])
+; CHECK-NEXT: [[TMP2:%.*]] = addrspacecast ptr addrspace(1) [[X]] to ptr addrspace(3)
+; CHECK-NEXT: [[TMP3:%.*]] = icmp eq ptr addrspace(3) [[TMP2]], null
+; CHECK-NEXT: [[TMP4:%.*]] = select i1 [[TMP1]], i1 true, i1 [[TMP3]]
+; CHECK-NEXT: ret i1 [[TMP4]]
+;
+ entry:
+ %0 = addrspacecast ptr addrspace(1) %x to ptr
+ %1 = tail call i1 @llvm.amdgcn.is.shared(ptr %0)
+ %2 = addrspacecast ptr %0 to ptr addrspace(3)
+ %3 = select i1 %1, ptr addrspace(3) null, ptr addrspace(3) %2
+ %4 = icmp eq ptr addrspace(3) %3, null
+ ret i1 %4
+}
|
|
@llvm/pr-subscribers-llvm-transforms Author: Wenju He (wenju-he) ChangesIn our downstream GPU target, following IR is valid before instcombine although the second addrspacecast causes UB. However, instcombine pass optimizes Full diff: https://github.com/llvm/llvm-project/pull/144686.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
index 73ba0f78e8053..a2335640f917b 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
@@ -3194,8 +3194,23 @@ static Instruction *foldNestedSelects(SelectInst &OuterSelVal,
/// Return true if V is poison or \p Expected given that ValAssumedPoison is
/// already poison. For example, if ValAssumedPoison is `icmp samesign X, 10`
/// and V is `icmp ne X, 5`, impliesPoisonOrCond returns true.
-static bool impliesPoisonOrCond(const Value *ValAssumedPoison, const Value *V,
- bool Expected) {
+static bool impliesPoisonOrCond(
+ const Value *ValAssumedPoison, const Value *V, bool Expected,
+ llvm::function_ref<bool(unsigned, unsigned)> isValidAddrSpaceCast) {
+ // Handle the case that ValAssumedPoison is `icmp eq ptr addrspace(3) X, null`
+ // and X is `addrspacecast ptr addrspace(1) Y to ptr addrspace(3)`. Target can
+ // replace X with poison if the addrspacecast is invalid. However, `V` might
+ // not be poison.
+ if (auto *ICmp = dyn_cast<ICmpInst>(ValAssumedPoison)) {
+ auto CanCreatePoison = [&](Value *Op) {
+ auto *ASC = dyn_cast<AddrSpaceCastInst>(Op);
+ return ASC && !isValidAddrSpaceCast(ASC->getDestAddressSpace(),
+ ASC->getSrcAddressSpace());
+ };
+ if (llvm::any_of(ICmp->operands(), CanCreatePoison))
+ return false;
+ }
+
if (impliesPoison(ValAssumedPoison, V))
return true;
@@ -3241,17 +3256,23 @@ Instruction *InstCombinerImpl::foldSelectOfBools(SelectInst &SI) {
auto *Zero = ConstantInt::getFalse(SelType);
Value *A, *B, *C, *D;
+ auto IsValidAddrSpaceCast = [&](unsigned FromAS, unsigned ToAS) {
+ return isValidAddrSpaceCast(FromAS, ToAS);
+ };
+
// Folding select to and/or i1 isn't poison safe in general. impliesPoison
// checks whether folding it does not convert a well-defined value into
// poison.
if (match(TrueVal, m_One())) {
- if (impliesPoisonOrCond(FalseVal, CondVal, /*Expected=*/false)) {
+ if (impliesPoisonOrCond(FalseVal, CondVal, /*Expected=*/false,
+ IsValidAddrSpaceCast)) {
// Change: A = select B, true, C --> A = or B, C
return BinaryOperator::CreateOr(CondVal, FalseVal);
}
if (match(CondVal, m_OneUse(m_Select(m_Value(A), m_One(), m_Value(B)))) &&
- impliesPoisonOrCond(FalseVal, B, /*Expected=*/false)) {
+ impliesPoisonOrCond(FalseVal, B, /*Expected=*/false,
+ IsValidAddrSpaceCast)) {
// (A || B) || C --> A || (B | C)
return replaceInstUsesWith(
SI, Builder.CreateLogicalOr(A, Builder.CreateOr(B, FalseVal)));
@@ -3287,13 +3308,15 @@ Instruction *InstCombinerImpl::foldSelectOfBools(SelectInst &SI) {
}
if (match(FalseVal, m_Zero())) {
- if (impliesPoisonOrCond(TrueVal, CondVal, /*Expected=*/true)) {
+ if (impliesPoisonOrCond(TrueVal, CondVal, /*Expected=*/true,
+ IsValidAddrSpaceCast)) {
// Change: A = select B, C, false --> A = and B, C
return BinaryOperator::CreateAnd(CondVal, TrueVal);
}
if (match(CondVal, m_OneUse(m_Select(m_Value(A), m_Value(B), m_Zero()))) &&
- impliesPoisonOrCond(TrueVal, B, /*Expected=*/true)) {
+ impliesPoisonOrCond(TrueVal, B, /*Expected=*/true,
+ IsValidAddrSpaceCast)) {
// (A && B) && C --> A && (B & C)
return replaceInstUsesWith(
SI, Builder.CreateLogicalAnd(A, Builder.CreateAnd(B, TrueVal)));
diff --git a/llvm/test/Transforms/InstCombine/AMDGPU/addrspacecast.ll b/llvm/test/Transforms/InstCombine/AMDGPU/addrspacecast.ll
new file mode 100644
index 0000000000000..4791d2c434884
--- /dev/null
+++ b/llvm/test/Transforms/InstCombine/AMDGPU/addrspacecast.ll
@@ -0,0 +1,23 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt -S -mtriple=amdgcn-amd-amdhsa -passes=instcombine %s | FileCheck %s
+
+; Check that `select B, true, C` isn't optimized to `or B, C`.
+define i1 @not_fold_select(ptr addrspace(1) noundef %x) {
+; CHECK-LABEL: define i1 @not_fold_select(
+; CHECK-SAME: ptr addrspace(1) noundef [[X:%.*]]) {
+; CHECK-NEXT: [[ENTRY:.*:]]
+; CHECK-NEXT: [[TMP0:%.*]] = addrspacecast ptr addrspace(1) [[X]] to ptr
+; CHECK-NEXT: [[TMP1:%.*]] = tail call i1 @llvm.amdgcn.is.shared(ptr [[TMP0]])
+; CHECK-NEXT: [[TMP2:%.*]] = addrspacecast ptr addrspace(1) [[X]] to ptr addrspace(3)
+; CHECK-NEXT: [[TMP3:%.*]] = icmp eq ptr addrspace(3) [[TMP2]], null
+; CHECK-NEXT: [[TMP4:%.*]] = select i1 [[TMP1]], i1 true, i1 [[TMP3]]
+; CHECK-NEXT: ret i1 [[TMP4]]
+;
+ entry:
+ %0 = addrspacecast ptr addrspace(1) %x to ptr
+ %1 = tail call i1 @llvm.amdgcn.is.shared(ptr %0)
+ %2 = addrspacecast ptr %0 to ptr addrspace(3)
+ %3 = select i1 %1, ptr addrspace(3) null, ptr addrspace(3) %2
+ %4 = icmp eq ptr addrspace(3) %3, null
+ ret i1 %4
+}
|
There was a problem hiding this comment.
If an invalid addrspacecast can produce poison, then this must be specified in
llvm-project/llvm/lib/Analysis/ValueTracking.cpp
Line 7352 in 34a4894
|
addrspacecast can produce poison. That's how we're handling the cases of addrspacecasts that are unimplementable |
| ; CHECK-NEXT: ret i1 [[TMP4]] | ||
| ; | ||
| entry: | ||
| %0 = addrspacecast ptr addrspace(1) %x to ptr |
| bool Expected) { | ||
| static bool impliesPoisonOrCond( | ||
| const Value *ValAssumedPoison, const Value *V, bool Expected, | ||
| llvm::function_ref<bool(unsigned, unsigned)> isValidAddrSpaceCast) { |
This PR uses isValidAddrSpaceCast query from TTI which is available in InstCombine. But TTI isn't available in above canCreateUndefOrPoison function.
done in ffff2c3, please review |
I don't think special handling of addrspacecast in |
llvm/docs/LangRef.rst
Outdated
| @@ -12621,6 +12621,9 @@ have no side effects, and must not capture the value of the pointer. | |||
| If the source is :ref:`poison <poisonvalues>`, the result is | |||
| :ref:`poison <poisonvalues>`. | |||
|
|
|||
| If the source is not :ref:`poison <poisonvalues>`, and the result pointer is | |||
| non-dereferenceable, the result is :ref:`poison <poisonvalues>`. | |||
There was a problem hiding this comment.
This is incorrect, not every addrspacecast of a non-dereferenceable pointer should automatically result in poison (e.g. the pointer to the end of an object is non-dereferenceable, but should certainly not turn into poison).
This should instead say something along the lines of:
Which address space casts are supported depends on the target. Unsupported address space casts return a poison value.
…butorAttributes to restore previous behavior in aapointer_info_map_invalidation.ll
llvmbot
added
the
llvm:analysis
thanks @dtcxzyw I've changed to return true for addrspacecast in Does it make sense to add an optional TTI argument to canCreateUndefOrPoison/isGuaranteedNotToBePoison/isGuaranteedNotToBeUndefOrPoison? If yes, then we don't need to query isValidAddrSpaceCast right after isGuaranteedNotToBeUndefOrPoison call in AttributorAttributes and potentially some other places. |
wenju-he
changed the title
| if (IRP.getPositionKind() != IRPosition::IRP_RETURNED && | ||
| isGuaranteedNotToBeUndefOrPoison(&Val)) { | ||
| (isGuaranteedNotToBeUndefOrPoison(&Val) || | ||
| IsTargetGuaranteedNotPoison(Val))) { |
There was a problem hiding this comment.
This is incorrect, it will not handle poison introduced higher up the chain if it ends in an addrspacecast. It's also sub-optimal, because it will not handle known-valid addrspacecasts higher up the chain. This needs to be handled fully inside canCreateUndefOrPoison.
|
For the purposes of this PR, I think you should only change canCreateUndefOrPoison to return true and just eat the regressions. We can follow up with using isValidAddrSpaceCast() in a followup, because it will be less straightforward. I'm not willing to accept direct use of TTI in ValueTracking without laundering it through an abstraction first (see https://discourse.llvm.org/t/constant-propagation-for-target-specific-intrinsics/85881/5). |
thanks @nikic |
done. Please let me know if we should add FIXME for the regression in clang/test/CodeGenOpenCL/amdgcn-buffer-rsrc-type.cl and llvm/test/Transforms/Attributor/reduced/aapointer_info_map_invalidation.ll The change in clang/test/CodeGenOpenCL/as_type.cl should not be a regression since spir isn't a concrete target. |
Co-authored-by: Nikita Popov <[email protected]>
| ; RUN: opt -S -mtriple=amdgcn-amd-amdhsa -passes=instcombine %s | FileCheck %s | ||
|
|
||
| ; Check that `select B, true, C` isn't optimized to `or B, C`, | ||
| ; because the invalid addrspacecast %asc.shared may introduce poison. |
There was a problem hiding this comment.
Description is slightly inaccurate. In this case it will always be poison
There was a problem hiding this comment.
Description is slightly inaccurate. In this case it will always be poison
done
In our downstream GPU target, following IR is valid before instcombine although the second addrspacecast causes UB.
define i1 @test(ptr addrspace(1) noundef %v) {
%0 = addrspacecast ptr addrspace(1) %v to ptr addrspace(4)
%1 = call i32 @llvm.xxxx.isaddr.shared(ptr addrspace(4) %0)
%2 = icmp eq i32 %1, 0
%3 = addrspacecast ptr addrspace(4) %0 to ptr addrspace(3)
%4 = select i1 %2, ptr addrspace(3) null, ptr addrspace(3) %3
%5 = icmp eq ptr addrspace(3) %4, null
ret i1 %5
}
We have a custom optimization that replaces invalid addrspacecast with poison, and IR is still valid since
selectstops poison propagation.However, instcombine pass optimizes
selecttoor:%0 = addrspacecast ptr addrspace(1) %v to ptr addrspace(4)
%1 = call i32 @llvm.xxxx.isaddr.shared(ptr addrspace(4) %0)
%2 = icmp eq i32 %1, 0
%3 = addrspacecast ptr addrspace(1) %v to ptr addrspace(3)
%4 = icmp eq ptr addrspace(3) %3, null
%5 = or i1 %2, %4
ret i1 %5
The transform is invalid for our target.