Update amdgpu_gfx functions to use s0-s3 for inreg SGPR arguments on targets using scratch instructions for stack #78553
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-backend-amdgpu Author: Rajveer Singh Bharadwaj (Rajveer100) ChangesResolves Issue #78226 Update the argument list for Full diff: https://github.com/llvm/llvm-project/pull/78553.diff 2 Files Affected:
diff --git a/llvm/lib/Target/AMDGPU/AMDGPUCallingConv.td b/llvm/lib/Target/AMDGPU/AMDGPUCallingConv.td
index c5207228dc913fe..4c922a81c02efd5 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUCallingConv.td
+++ b/llvm/lib/Target/AMDGPU/AMDGPUCallingConv.td
@@ -23,6 +23,7 @@ def CC_SI_Gfx : CallingConv<[
// 33 is reserved for the frame pointer
// 34 is reserved for the base pointer
CCIfInReg<CCIfType<[f32, i32, f16, i16, v2i16, v2f16, bf16, v2bf16] , CCAssignToReg<[
+ SGPR0, SGPR1, SGPR2, SGPR3,
SGPR4, SGPR5, SGPR6, SGPR7,
SGPR8, SGPR9, SGPR10, SGPR11, SGPR12, SGPR13, SGPR14, SGPR15,
SGPR16, SGPR17, SGPR18, SGPR19, SGPR20, SGPR21, SGPR22, SGPR23,
diff --git a/llvm/lib/Target/AMDGPU/SIISelLowering.cpp b/llvm/lib/Target/AMDGPU/SIISelLowering.cpp
index 3852f93da98dc45..2a5cd441d6e215f 100644
--- a/llvm/lib/Target/AMDGPU/SIISelLowering.cpp
+++ b/llvm/lib/Target/AMDGPU/SIISelLowering.cpp
@@ -2786,7 +2786,7 @@ SDValue SITargetLowering::LowerFormalArguments(
if (!IsKernel) {
CCAssignFn *AssignFn = CCAssignFnForCall(CallConv, isVarArg);
- if (!IsGraphics && !Subtarget->enableFlatScratch()) {
+ if (!Subtarget->enableFlatScratch()) {
CCInfo.AllocateRegBlock(ArrayRef<MCPhysReg>{AMDGPU::SGPR0, AMDGPU::SGPR1,
AMDGPU::SGPR2, AMDGPU::SGPR3},
4);
|
|
@arsenm PS: Will update the tests. Test Details |
|
@philnik777 @ldionne |
|
How would I know? I'm working on libc++ and clang, not LLVM backends. |
|
Code change looks right (though it's missing the mirror change for GlobalISel in AMDGPUCallLowering). This will need test updates though |
Rajveer100
force-pushed
the
branch-for-issue-78226
branch
from
4f341fe
to
816cac6
Compare
arsenm
reviewed
Jan 26, 2024
arsenm
requested changes
Feb 6, 2024
Rajveer100
force-pushed
the
branch-for-issue-78226
branch
from
816cac6
to
a78f4ae
Compare
|
How should the tests be added, I see too many of them failing at the moment? |
This should be covered by existing tests (which just now need many updates) |
…targets using scratch instructions for stack Resolves Issue llvm#78226
|
I don't think I have to manually fix each of the 175 tests individually, or do I? Also, could you describe about testing .ll files in particular that have FileCheck and assertions. |
Everything that's failing. 175 sounds a bit high for this. Just about every test has FileCheck. update_llc_test_checks ----update-only will likely get most of them |
|
Do I have to include any additional builds (like openmp) in the llvm build to generate those tests, as they are currently only in the llvm/ directory and hence isn't invoked by update_llc_test_checks due to this? > llvm/utils/update_llc_test_checks.py llvm/test/CodeGen/AMDGPU/GlobalISel/irtranslator-call.ll -u
WARNING: Skipping test which wasn't autogenerated by utils/update_llc_test_checks.py: llvm/test/CodeGen/AMDGPU/GlobalISel/irtranslator-call.ll |
Rajveer100
force-pushed
the
branch-for-issue-78226
branch
from
a78f4ae
to
f72151c
Compare
No
Some are updated by update_mir_test_checks, some update_test_checks, and some are hand written. Some have checks for multiple types of checks (which the update scripts don't gracefully handle). I'd expect update_llc_test_checks to handle the majority of them |
|
For clarity, I see this warning for all tests, i.e, I am unable to use any of those utility commands with the same issue as mentioned in the above comment. |
@arsenm |
Some tests are not autogenerated and will need manually updating (or updating with update_mir_test_checks). The warning is expected. You should see it working on some of them Your example llvm/test/CodeGen/AMDGPU/GlobalISel/irtranslator-call.ll should use update_mir_test_checks |
|
Duplicate #81394 |
|
@arsenm Will keep an eye on the same, to learn more! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves Issue #78226
Update the argument list for
*_Gfxcalling conventions for AMDGPU calling convention and update the conditional check underenableFlatScratchwhich pre-allocates the registers.