Skip to content

[AMDGPU] Modify Dyn Alloca test to account for Machine-Verifier bug #120393

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

Merged
merged 1 commit into from
Dec 18, 2024

Conversation

easyonaadit
Copy link
Contributor

@easyonaadit easyonaadit commented Dec 18, 2024

Machine-Verifier crashes in kernel functions,
but fails gracefully in device functions.

This is due to the buffer resource descriptor selected
during G-ISEL, before the fallback path.
Device functions use $sgpr0_sgpr1_sgpr2_sgpr3.
while Kernel functions select $private_rsrc_reg
where machine-verifier complains:
$private_rsrc_reg is not a SReg_128 register.

Modifying test case to capture both behaviors, this is related to #120063

@easyonaadit easyonaadit changed the title [AMDGPU] Modify Dyn Alloca test case to account for Machine-Verifier behavior [AMDGPU] Modify Dyn Alloca tests to account for Machine-Verifier behavior Dec 18, 2024
@easyonaadit easyonaadit changed the title [AMDGPU] Modify Dyn Alloca tests to account for Machine-Verifier behavior [AMDGPU] Modify Dyn Alloca test to account for Machine-Verifier behavior Dec 18, 2024
@easyonaadit easyonaadit marked this pull request as ready for review December 18, 2024 09:44
@llvmbot
Copy link
Member

llvmbot commented Dec 18, 2024

@llvm/pr-subscribers-llvm-globalisel

@llvm/pr-subscribers-backend-amdgpu

Author: Aaditya (easyonaadit)

Changes

Machine-Verifier crashes in kernel functions,
but fails gracefully in device functions.

This is due to the buffer resource descriptor selected
during G-ISEL, before the fallback path.
Device functions use $sgpr0_sgpr1_sgpr2_sgpr3.
while Kernel functions select $private_rsrc_reg
where machine-verifier complains:
$private_rsrc_reg is not a SReg_128 register.

Modifying test case to capture both behaviors, this is related to #120063


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

1 Files Affected:

  • (modified) llvm/test/CodeGen/AMDGPU/GlobalISel/dynamic-alloca-divergent.ll (+12-7)
diff --git a/llvm/test/CodeGen/AMDGPU/GlobalISel/dynamic-alloca-divergent.ll b/llvm/test/CodeGen/AMDGPU/GlobalISel/dynamic-alloca-divergent.ll
index 5dae7885f6bfb1..e0f22bc32b63c8 100644
--- a/llvm/test/CodeGen/AMDGPU/GlobalISel/dynamic-alloca-divergent.ll
+++ b/llvm/test/CodeGen/AMDGPU/GlobalISel/dynamic-alloca-divergent.ll
@@ -1,25 +1,30 @@
-; RUN: not llc -global-isel -mtriple=amdgcn-amd-amdhsa -mcpu=gfx900 -global-isel-abort=2 -pass-remarks-missed="gisel.*" -verify-machineinstrs -o /dev/null 2>&1 %s | FileCheck -check-prefix=ERR %s
+; RUN: not --crash llc -global-isel -mtriple=amdgcn-amd-amdhsa -mcpu=gfx900 -global-isel-abort=2 -pass-remarks-missed="gisel.*" -verify-machineinstrs -o /dev/null 2>&1 %s | FileCheck -check-prefix=ERR_KERNEL %s
+; RUN: not llc -global-isel -mtriple=amdgcn-amd-amdhsa -mcpu=gfx900 -global-isel-abort=2 -pass-remarks-missed="gisel.*" -o /dev/null 2>&1 %s | FileCheck -check-prefix=ERR %s
+
+; ERR_KERNEL: remark: <unknown>:0:0: cannot select: %{{[0-9]+}}:sreg_32(p5) = G_DYN_STACKALLOC %{{[0-9]+}}:vgpr(s32), 1 (in function: kernel_dynamic_stackalloc_vgpr_align4)
+; ERR_KERNEL-NEXT: warning: Instruction selection used fallback path for kernel_dynamic_stackalloc_vgpr_align4
+; ERR_KERNEL-NEXT: error: <unknown>:0:0: in function kernel_dynamic_stackalloc_vgpr_align4 void (ptr addrspace(1)): unsupported dynamic alloca
 
 ; ERR: remark: <unknown>:0:0: cannot select: %{{[0-9]+}}:sreg_32(p5) = G_DYN_STACKALLOC %{{[0-9]+}}:vgpr(s32), 1 (in function: kernel_dynamic_stackalloc_vgpr_align4)
 ; ERR-NEXT: warning: Instruction selection used fallback path for kernel_dynamic_stackalloc_vgpr_align4
 ; ERR-NEXT: error: <unknown>:0:0: in function kernel_dynamic_stackalloc_vgpr_align4 void (ptr addrspace(1)): unsupported dynamic alloca
 
-; ERR: remark: <unknown>:0:0: cannot select: %{{[0-9]+}}:sreg_32(p5) = G_DYN_STACKALLOC %{{[0-9]+}}:vgpr(s32), 1 (in function: func_dynamic_stackalloc_vgpr_align4)
-; ERR-NEXT: warning: Instruction selection used fallback path for func_dynamic_stackalloc_vgpr_align4
-; ERR-NEXT: error: <unknown>:0:0: in function func_dynamic_stackalloc_vgpr_align4 void (i32): unsupported dynamic alloca
-
 define amdgpu_kernel void @kernel_dynamic_stackalloc_vgpr_align4(ptr addrspace(1) %ptr) {
   %id = call i32 @llvm.amdgcn.workitem.id.x()
   %gep = getelementptr i32, ptr addrspace(1) %ptr, i32 %id
   %n = load i32, ptr addrspace(1) %gep
   %alloca = alloca i32, i32 %n, align 4, addrspace(5)
-  store volatile ptr addrspace(5) %alloca, ptr addrspace(1) undef
+  store volatile i32 5, ptr addrspace(5) %alloca
   ret void
 }
 
+; ERR: remark: <unknown>:0:0: cannot select: %{{[0-9]+}}:sreg_32(p5) = G_DYN_STACKALLOC %{{[0-9]+}}:vgpr(s32), 1 (in function: func_dynamic_stackalloc_vgpr_align4)
+; ERR-NEXT: warning: Instruction selection used fallback path for func_dynamic_stackalloc_vgpr_align4
+; ERR-NEXT: error: <unknown>:0:0: in function func_dynamic_stackalloc_vgpr_align4 void (i32): unsupported dynamic alloca
+
 define void @func_dynamic_stackalloc_vgpr_align4(i32 %n) {
   %alloca = alloca i32, i32 %n, align 4, addrspace(5)
-  store volatile ptr addrspace(5) %alloca, ptr addrspace(1) undef
+  store volatile i32 10, ptr addrspace(5) %alloca
   ret void
 }
 

@arsenm
Copy link
Contributor

arsenm commented Dec 18, 2024

his is due to the buffer resource descriptor selected
during G-ISEL, before the fallback path.
Device functions use $sgpr0_sgpr1_sgpr2_sgpr3.
while Kernel functions select $private_rsrc_reg
where machine-verifier complains:
$private_rsrc_reg is not a SReg_128 register.

This is just a bug that should be fixed, not a behavior to consider

@easyonaadit
Copy link
Contributor Author

This is just a bug that should be fixed, not a behavior to consider

I need to modify the run line of this test file before pre-committing other tests for dynamic allocas. #120063
Another option is to remove the -verify-machineinstrs flag from the run line until this bug is resolved.

@easyonaadit easyonaadit changed the title [AMDGPU] Modify Dyn Alloca test to account for Machine-Verifier behavior [AMDGPU] Modify Dyn Alloca test to account for Machine-Verifier bug Dec 18, 2024
@arsenm
Copy link
Contributor

arsenm commented Dec 18, 2024

This is just a bug that should be fixed, not a behavior to consider

I need to modify the run line of this test file before pre-committing other tests for dynamic allocas. #120063 Another option is to remove the -verify-machineinstrs flag from the run line until this bug is resolved.

You would need an explicit -verify-machineinstrs=0 to avoid breaking EXPENSIVE_CHECKS builds

@easyonaadit easyonaadit force-pushed the reapply-pre-commit-test-cases branch from 7e44fba to c01c1bb Compare December 18, 2024 10:16
@easyonaadit
Copy link
Contributor Author

easyonaadit commented Dec 18, 2024

You would need an explicit -verify-machineinstrs=0 to avoid breaking EXPENSIVE_CHECKS builds

yess have done that instead.

Copy link
Contributor

@arsenm arsenm left a comment

Choose a reason for hiding this comment

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

we ought to fix the machine verifier issue on fallback

@easyonaadit
Copy link
Contributor Author

we ought to fix the machine verifier issue on fallback

Right, I'll be able to look into it after the dynamic alloca patch lands.

@pravinjagtap pravinjagtap merged commit 414c462 into llvm:main Dec 18, 2024
5 of 7 checks passed
lalaniket8 pushed a commit that referenced this pull request Dec 18, 2024
…locas" (#120410)

This reapplies commit #120063.

A machine-verifier bug was causing a crash in the previous commit. 
This has been addressed in
#120393.
github-actions bot pushed a commit to arm/arm-toolchain that referenced this pull request Jan 10, 2025
… dynamic allocas" (#120410)

This reapplies commit llvm/llvm-project#120063.

A machine-verifier bug was causing a crash in the previous commit.
This has been addressed in
llvm/llvm-project#120393.
searlmc1 pushed a commit to ROCm/llvm-project that referenced this pull request Mar 24, 2025
…lvm#120393)

Machine-Verifier crashes in kernel functions, 
but fails gracefully in device functions.

This is due to the buffer resource descriptor selected
during G-ISEL, before the fallback path. 
Device functions use `$sgpr0_sgpr1_sgpr2_sgpr3`.
while Kernel functions select `$private_rsrc_reg` 
where machine-verifier complains: 
`$private_rsrc_reg is not a SReg_128 register.`

Modifying test case to capture both behaviors, this is related to
llvm#120063
searlmc1 pushed a commit to ROCm/llvm-project that referenced this pull request Mar 24, 2025
…locas" (llvm#120410)

This reapplies commit llvm#120063.

A machine-verifier bug was causing a crash in the previous commit. 
This has been addressed in
llvm#120393.
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.

4 participants