Skip to content

[AMDGPU][NFC] Added test for live-in CSR SGPR used partially giving MachineVerifier error #126696

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 3 commits into from
Feb 25, 2025

Conversation

vg0204
Copy link
Contributor

@vg0204 vg0204 commented Feb 11, 2025

No description provided.

@vg0204 vg0204 self-assigned this Feb 11, 2025
@vg0204 vg0204 marked this pull request as draft February 11, 2025 08:23
@llvmbot
Copy link
Member

llvmbot commented Feb 11, 2025

@llvm/pr-subscribers-backend-amdgpu

Author: Vikash Gupta (vg0204)

Changes

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

1 Files Affected:

  • (added) llvm/test/CodeGen/AMDGPU/spill-partial-csr-sgpr-live-ins.mir (+52)
diff --git a/llvm/test/CodeGen/AMDGPU/spill-partial-csr-sgpr-live-ins.mir b/llvm/test/CodeGen/AMDGPU/spill-partial-csr-sgpr-live-ins.mir
new file mode 100644
index 00000000000000..8ed88339d59181
--- /dev/null
+++ b/llvm/test/CodeGen/AMDGPU/spill-partial-csr-sgpr-live-ins.mir
@@ -0,0 +1,52 @@
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -mtriple=amdgcn-amd-amdhsa -verify-machineinstrs -run-pass=si-lower-sgpr-spills -o - %s | FileCheck %s
+# XFAIL: *
+
+# FIXME : Currently, MRI's liveIn check for regiters does not take corresponding live-in's sub-registers in account. As a result
+# in SILowerSGPRSpills, the SubReg spill gets marked KILLED eventhough its SuperReg is in function Live-ins. This causes machine
+# verifier to now fail at direct usage of that SubReg, which intially should not be any problem before adding spill.
+
+# CHECK: *** Bad machine code: Using an undefined physical register ***
+
+---
+name: spill_partial_live_csr_sgpr_argument_test_1
+tracksRegLiveness: true
+liveins:
+  - { reg: '$sgpr50_sgpr51' }
+body:             |
+  bb.0:
+    liveins: $sgpr50_sgpr51
+
+    S_NOP 0, implicit $sgpr50
+    $sgpr50 = S_MOV_B32 0
+
+...
+
+---
+name: spill_partial_live_csr_sgpr_argument_test_2
+tracksRegLiveness: true
+liveins:
+  - { reg: '$sgpr50_sgpr51' }
+body:             |
+  bb.0:
+    liveins: $sgpr50_sgpr51
+
+    S_NOP 0, implicit $sgpr50
+    $sgpr50_sgpr51 = S_MOV_B64 0
+
+...
+
+---
+name: spill_partial_live_csr_sgpr_argument_test_3
+tracksRegLiveness: true
+liveins:
+  - { reg: '$sgpr52_sgpr53' }
+body:             |
+  bb.0:
+    liveins: $sgpr52_sgpr53
+
+    S_NOP 0, implicit $sgpr53
+    $sgpr52_sgpr53 = S_MOV_B64 0
+    $sgpr54 = S_MOV_B32 0
+
+...

@vg0204 vg0204 requested review from arsenm and cdevadas February 11, 2025 08:23
@vg0204 vg0204 marked this pull request as ready for review February 11, 2025 09:20
@vg0204 vg0204 changed the title [AMDGPU][LIT][NFC] Added xfail test for live-in CSR SGPR used partially [AMDGPU][LIT][NFC] Added test for live-in CSR SGPR used partially giving MachineVerifier error Feb 11, 2025
@vg0204
Copy link
Contributor Author

vg0204 commented Feb 19, 2025

@arsenm, @jayfoad ping!

@shiltian shiltian changed the title [AMDGPU][LIT][NFC] Added test for live-in CSR SGPR used partially giving MachineVerifier error [AMDGPU][NFC] Added test for live-in CSR SGPR used partially giving MachineVerifier error Feb 19, 2025
@vg0204
Copy link
Contributor Author

vg0204 commented Feb 24, 2025

@arsenm , @cdevadas , @jayfoad , can anyone approve this if its alright, so I can continue with the patch PR, thanks!

Copy link
Contributor

@jayfoad jayfoad left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +24 to +29
liveins:
- { reg: '$sgpr50_sgpr51' }
- { reg: '$sgpr52_sgpr53' }
- { reg: '$sgpr54_sgpr55' }
Copy link
Contributor

Choose a reason for hiding this comment

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

Should drop the function live ins part. I don't believe it should be required for any liveness functionality

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needed for MRI's livein check as done in SILowerSGPRSpills.

Copy link
Contributor

Choose a reason for hiding this comment

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

SILowerSGPRSpills should not be depending on this information, and it is not being used in this test

@cdevadas
Copy link
Collaborator

Can you stack the PR #126926 along with it? This PR is supposed to fix the error you mentioned in this test. We should be able to see how the proposed fix changes the test if they are stacked together.

@vg0204
Copy link
Contributor Author

vg0204 commented Feb 25, 2025

Can you stack the PR #126926 along with it? This PR is supposed to fix the error you mentioned in this test. We should be able to see how the proposed fix changes the test if they are stacked together.

In order to stack PRs (assuming graphite), don't we need to create new PRs completely for test & patch as both of these branches as for now are independent & created from my forked repo's main. I am thinking for this time to merge it in upstream & rebase it for the patch PR, that might do the trick for now. Will take care of stacking in such future cases!

@vg0204 vg0204 force-pushed the vg0204/xfail-mir-test-sgpr-csr-spill branch from e7aaf4a to d1e3339 Compare February 25, 2025 06:59
@pravinjagtap
Copy link
Contributor

pravinjagtap commented Feb 25, 2025 via email

@vg0204 vg0204 merged commit 49f60b4 into llvm:main Feb 25, 2025
11 checks passed
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.

7 participants