[AMDGPU] Quit PromoteAllocaToVector if intrinsic is used #68744
Conversation
| @@ -771,7 +771,8 @@ bool AMDGPUPromoteAllocaImpl::tryPromoteAllocaToVector(AllocaInst &Alloca) { | |||
| } | |||
|
|
|||
| // Ignore assume-like intrinsics and comparisons used in assumes. | |||
| if (isAssumeLikeIntrinsic(Inst)) { | |||
| // Make sure that intrinsics do not have any use like e.g. llvm.objectsize | |||
| if (isAssumeLikeIntrinsic(Inst) && Inst->use_empty()) { | |||
There was a problem hiding this comment.
This change will fix the crash, but I'm not sure about it overall.
We have at least three intrinsics from isAssumeLikeIntrinsic which are returning something.
objectsize, invariant.start and ptr.annotation. Maybe we should allow objectsize to pass this check and add objectsize to the WorkList and later in promoteAllocaUserToVector we could just calculate the size of the alloca ?. If IR contains invariant.start or ptr.annotation we will just quit.
There was a problem hiding this comment.
Instead of this you should do
if(isAssumeLikeIntrinsic(Inst)) {
if(!Inst->use_empty()) {
RejectUserr(Inst, "assume-like intrinsic cannot have any users");
return false;
}
To explicitly reject them IMO.
There was a problem hiding this comment.
Can you rename the change to something like "Bail if assume-like intrinsic is used in PromoteAllocaToVector"? Current title sounds like you want to disallow intrinsics altogether :)
About the change itself I have no strong opinion, I'm not familiar with those intrinsics so I added other reviewers. I'm wondering if we could maybe pre-calculate the value of those intrinsics and replace them? If the value is just sizeof we can replace it with a ConstantInt of the size of the VectorType for instance?
It all depends on how common these are. If they're very rare then we can just ignore them I think, and only implement better handling if we have data to prove we need to do better with them.
| @@ -771,7 +771,8 @@ bool AMDGPUPromoteAllocaImpl::tryPromoteAllocaToVector(AllocaInst &Alloca) { | |||
| } | |||
|
|
|||
| // Ignore assume-like intrinsics and comparisons used in assumes. | |||
| if (isAssumeLikeIntrinsic(Inst)) { | |||
| // Make sure that intrinsics do not have any use like e.g. llvm.objectsize | |||
| if (isAssumeLikeIntrinsic(Inst) && Inst->use_empty()) { | |||
There was a problem hiding this comment.
Instead of this you should do
if(isAssumeLikeIntrinsic(Inst)) {
if(!Inst->use_empty()) {
RejectUserr(Inst, "assume-like intrinsic cannot have any users");
return false;
}
To explicitly reject them IMO.
mariusz-sikora-at-amd
force-pushed
the
masikora/pub-PromoteAllocaToVectorFixObjectSizeCrash
branch
from
17d6279 to
d476222
Compare
|
@llvm/pr-subscribers-backend-amdgpu Author: Mariusz Sikora (mariusz-sikora-at-amd) ChangesAttached test will cause crash without this change. We should not remove isAssumeLikeIntrinsic instruction if it is used by other instruction. Full diff: https://github.com/llvm/llvm-project/pull/68744.diff 2 Files Affected:
diff --git a/llvm/lib/Target/AMDGPU/AMDGPUPromoteAlloca.cpp b/llvm/lib/Target/AMDGPU/AMDGPUPromoteAlloca.cpp
index 3707a960211eb49..6f5791fecdaf54c 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUPromoteAlloca.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPUPromoteAlloca.cpp
@@ -772,6 +772,8 @@ bool AMDGPUPromoteAllocaImpl::tryPromoteAllocaToVector(AllocaInst &Alloca) {
// Ignore assume-like intrinsics and comparisons used in assumes.
if (isAssumeLikeIntrinsic(Inst)) {
+ if (!Inst->use_empty())
+ return RejectUser(Inst, "assume-line intrinsic cannot have any users");
UsersToRemove.push_back(Inst);
continue;
}
diff --git a/llvm/test/CodeGen/AMDGPU/promote-alloca-mem-intrinsics.ll b/llvm/test/CodeGen/AMDGPU/promote-alloca-mem-intrinsics.ll
index 0bba1bdce95655b..5616bc0f5ef3c12 100644
--- a/llvm/test/CodeGen/AMDGPU/promote-alloca-mem-intrinsics.ll
+++ b/llvm/test/CodeGen/AMDGPU/promote-alloca-mem-intrinsics.ll
@@ -53,6 +53,15 @@ define amdgpu_kernel void @promote_with_objectsize(ptr addrspace(1) %out) #0 {
ret void
}
+; CHECK-LABEL: @promote_with_objectsize_8(
+; CHECK: [[PTR:%[0-9]+]] = getelementptr inbounds [64 x [8 x i32]], ptr addrspace(3) @promote_with_objectsize_8.alloca, i32 0, i32 %{{[0-9]+}}
+; CHECK: call i32 @llvm.objectsize.i32.p3(ptr addrspace(3) [[PTR]], i1 false, i1 false, i1 false)
+define amdgpu_kernel void @promote_with_objectsize_8(ptr addrspace(1) %out) #0 {
+ %alloca = alloca [8 x i32], align 4, addrspace(5)
+ %size = call i32 @llvm.objectsize.i32.p5(ptr addrspace(5) %alloca, i1 false, i1 false, i1 false)
+ store i32 %size, ptr addrspace(1) %out
+ ret void
+}
; CHECK-LABEL: @promote_alloca_used_twice_in_memcpy(
; CHECK: call void @llvm.memcpy.p3.p3.i64(ptr addrspace(3) align 8 dereferenceable(16) %arrayidx1, ptr addrspace(3) align 8 dereferenceable(16) %arrayidx2, i64 16, i1 false)
define amdgpu_kernel void @promote_alloca_used_twice_in_memcpy(i32 %c) {
|
Attached test will cause crash without this change.
mariusz-sikora-at-amd
force-pushed
the
masikora/pub-PromoteAllocaToVectorFixObjectSizeCrash
branch
from
d476222 to
3dc3a43
Compare
|
ping |
mariusz-sikora-at-amd
deleted the
masikora/pub-PromoteAllocaToVectorFixObjectSizeCrash
branch
Attached test will cause crash without this change.
We should not remove isAssumeLikeIntrinsic instruction if it is used by other instruction.