[AMDGPU] Fix register class constraints for si-fold-operands pass when folding immediate into copies #131387
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-backend-amdgpu Author: None (mssefat) ChangesThis fixes an issue where the si-fold-operands pass would incorrectly fold immediate values into COPY instructions targeting av_32 registers. The pass now checks register class constraints before attempting to fold the immediate. Full diff: https://github.com/llvm/llvm-project/pull/131387.diff 2 Files Affected:
diff --git a/llvm/lib/Target/AMDGPU/SIFoldOperands.cpp b/llvm/lib/Target/AMDGPU/SIFoldOperands.cpp
index 91df516b80857..5a7fefaafd768 100644
--- a/llvm/lib/Target/AMDGPU/SIFoldOperands.cpp
+++ b/llvm/lib/Target/AMDGPU/SIFoldOperands.cpp
@@ -1047,6 +1047,11 @@ void SIFoldOperandsImpl::foldOperand(
if (MovOp == AMDGPU::COPY)
return;
+ // Check if the destination register of the MOV operation belongs
+ // to a vector superclass. Folding would be illegal.
+ if (TRI->isVectorSuperClass(DestRC))
+ return;
+
MachineInstr::mop_iterator ImpOpI = UseMI->implicit_operands().begin();
MachineInstr::mop_iterator ImpOpE = UseMI->implicit_operands().end();
while (ImpOpI != ImpOpE) {
diff --git a/llvm/test/CodeGen/AMDGPU/si-fold-operands-imm-into-copy.mir b/llvm/test/CodeGen/AMDGPU/si-fold-operands-imm-into-copy.mir
new file mode 100644
index 0000000000000..869da156a83d6
--- /dev/null
+++ b/llvm/test/CodeGen/AMDGPU/si-fold-operands-imm-into-copy.mir
@@ -0,0 +1,36 @@
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 5
+# RUN: llc -mtriple=amdgcn -mcpu=gfx90a -run-pass=si-fold-operands -verify-machineinstrs -o - %s 2>&1 | FileCheck %s
+
+---
+
+name: s_mov_b32_imm_literal_copy_s_to_av_32
+tracksRegLiveness: true
+body: |
+ bb.0:
+ ; CHECK-LABEL: name: s_mov_b32_imm_literal_copy_s_to_av_32
+ ; CHECK: [[S_MOV_B32_:%[0-9]+]]:sreg_32 = S_MOV_B32 999
+ ; CHECK-NEXT: [[COPY:%[0-9]+]]:av_32 = COPY [[S_MOV_B32_]]
+ ; CHECK-NEXT: $agpr0 = COPY [[COPY]]
+ ; CHECK-NEXT: S_ENDPGM 0
+ %0:sreg_32 = S_MOV_B32 999
+ %1:av_32 = COPY %0
+ $agpr0 = COPY %1
+ S_ENDPGM 0
+...
+
+---
+
+name: v_mov_b32_imm_literal_copy_v_to_av_32
+tracksRegLiveness: true
+body: |
+ bb.0:
+ ; CHECK-LABEL: name: v_mov_b32_imm_literal_copy_v_to_av_32
+ ; CHECK: [[V_MOV_B32_e32_:%[0-9]+]]:vgpr_32 = V_MOV_B32_e32 999, implicit $exec
+ ; CHECK-NEXT: [[COPY:%[0-9]+]]:av_32 = COPY [[V_MOV_B32_e32_]]
+ ; CHECK-NEXT: $agpr0 = COPY [[COPY]]
+ ; CHECK-NEXT: S_ENDPGM 0
+ %0:vgpr_32 = V_MOV_B32_e32 999, implicit $exec
+ %1:av_32 = COPY %0
+ $agpr0 = COPY %1
+ S_ENDPGM 0
+...
|
|
@arsenm could you please review? |
|
The tests for the issue are already in llvm/test/CodeGen/AMDGPU/fold-imm-copy.mir (and you should also mention this fixes issue number in the description) |
mssefat
force-pushed
the
users/mssefat/si-fold-operands-copy-imm
branch
from
fe1800a to
30124cf
Compare
|
@arsenm I updated accordingly. Please check. |
|
|
||
| # ... | ||
| --- | ||
| name: s_mov_b32_inlineimm_copy_s_to_av_32 |
There was a problem hiding this comment.
New tests need checks for the expected output. Maybe convert the while file to use update_mir_test_checks?
There was a problem hiding this comment.
I have pushed the updated changes. Could you please review?
| // Check if the destination register of the MOV operation belongs | ||
| // to a vector superclass. Folding would be illegal. | ||
| if (TRI->isVectorSuperClass(DestRC)) | ||
| return; |
There was a problem hiding this comment.
Would be better to query the register class of the result register, and check if there is a common class between it and the virtual register class. In the future it would be better to replace the instruction and modify the register class to enable folds in some cases
There was a problem hiding this comment.
Thanks for your suggestion. Based on your comment, I've implemented the check. The snippet looks like following:
// Check if the destination register class has a common class
// with the expected register class for the MOV instruction.
const MCInstrDesc &MovDesc = TII->get(MovOp);
if (MovDesc.operands()[0].RegClass != -1) {
const TargetRegisterClass *ResRC =
TRI->getRegClass(MovDesc.operands()[0].RegClass);
const TargetRegisterClass *CommonRC =
TRI->getCommonSubClass(DestRC, ResRC);
if (!CommonRC) {
return;
}
}
Can you please confirm if this aligns correctly with your suggestion or further adjustments are needed?
There was a problem hiding this comment.
I have pushed the updated changes. Could you please review?
mssefat
force-pushed
the
users/mssefat/si-fold-operands-copy-imm
branch
from
30124cf to
d2316ee
Compare
| // is a superclass of (or equal to) the destination register class of the COPY (DestRC). | ||
| // If this condition fails, folding would be illegal. | ||
| const MCInstrDesc &MovDesc = TII->get(MovOp); | ||
| if (MovDesc.getNumDefs() > 0 && MovDesc.operands()[0].RegClass != -1) { |
There was a problem hiding this comment.
Can you just assert these conditions? I don't think they should ever fail.
There was a problem hiding this comment.
Done, can you please review?
| if (MovDesc.getNumDefs() > 0 && MovDesc.operands()[0].RegClass != -1) { | ||
| const TargetRegisterClass *ResRC = | ||
| TRI->getRegClass(MovDesc.operands()[0].RegClass); | ||
| if (!DestRC -> hasSuperClassEq(ResRC)) return; |
There was a problem hiding this comment.
No space around ->. Body of if on separate line.
| if (!DestRC -> hasSuperClassEq(ResRC)) return; | |
| if (!DestRC->hasSuperClassEq(ResRC)) | |
| return; |
There was a problem hiding this comment.
Can you use getCommonSubClass or getMatchingSuperRegClass instead of directly checking hasSuperClassEq
There was a problem hiding this comment.
I have pushed the updated changes. Could you please review?
|
|
||
| # GCN-LABEL: name: fold-imm-copy | ||
| # GCN: V_AND_B32_e32 65535 | ||
| # NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 5 |
There was a problem hiding this comment.
Please make a separate PR for this and commit it first. Then rebase the current PR.
There was a problem hiding this comment.
Done and I have pushed the updated changes. Could you please review?
Fixes llvm#130020 This fixes an issue where the si-fold-operands pass would incorrectly fold immediate values into COPY instructions targeting av_32 registers, which is illegal. The pass now properly checks register class constraints before attempting to fold the immediates.
This fixes an issue where the si-fold-operands pass would incorrectly fold immediate values into COPY instructions targeting av_32 registers, which is illegal. The pass now properly checks register class constraints before attempting to fold the immediates.
mssefat
force-pushed
the
users/mssefat/si-fold-operands-copy-imm
branch
from
d2316ee to
547d52f
Compare
Fixes llvm#130020 This fixes an issue where the si-fold-operands pass would incorrectly fold immediate values into COPY instructions targeting av_32 registers, which is illegal. The pass now properly checks register class constraints before attempting to fold the immediates.
| if (!CommonRC || !DestRC->hasSuperClassEq(CommonRC)) | ||
| return; | ||
|
|
There was a problem hiding this comment.
| if (!CommonRC || !DestRC->hasSuperClassEq(CommonRC)) | |
| return; | |
| if (!CommonRC) | |
| return; | |
hasSuperClassEq should be redundant with finding the common class
There was a problem hiding this comment.
I think hasSuperClassEq is not redundant with finding common subclass. Consider this basic block where the copy operation is illegal:
bb.0:
%1:av_32 = V_MOV_B32_e32 32, implicit $exec
$agpr0 = COPY %1:av_32
S_ENDPGM 0
In this example:
DestRC is AV_32
ResRC is VGPR_32
CommonRC is VGPR_32
If we only check for CommonRC, the check would allow folding in this case since CommonRC exists (VGPR_32). However, folding here is illegal. And I think we need the condition if (!DestRC->hasSuperClassEq(CommonRC)) return;.
There was a problem hiding this comment.
Why do you need CommonRC? Isn't the whole check equivalent to DestRC->hasSuperClassEq(ResRC)?
There was a problem hiding this comment.
The code as you have written it is weird, because CommonRC is by construction a subclass of DestRC, and then you are checking that it is a superclass of DestRC. These can only both be true if CommonRC is DestRC.
There was a problem hiding this comment.
They should be equivalent check. I am reverting to the previous use of DestRC->hasSuperClassEq(ResRC). @arsenm please let me know if I am missing something.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
@mssefat Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/137/builds/17778 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/190/builds/19286 Here is the relevant piece of the build log for the reference |
…n folding immediate into copies (llvm#131387) Fixes llvm#130020 This fixes an issue where the si-fold-operands pass would incorrectly fold immediate values into COPY instructions targeting av_32 registers. The pass now checks register class constraints before attempting to fold the immediate.
…n folding immediate into copies (llvm#131387) Fixes llvm#130020 This fixes an issue where the si-fold-operands pass would incorrectly fold immediate values into COPY instructions targeting av_32 registers. The pass now checks register class constraints before attempting to fold the immediate.
…n folding immediate into copies (llvm#131387) Fixes llvm#130020 This fixes an issue where the si-fold-operands pass would incorrectly fold immediate values into COPY instructions targeting av_32 registers. The pass now checks register class constraints before attempting to fold the immediate.
…n folding immediate into copies (llvm#131387) Fixes llvm#130020 This fixes an issue where the si-fold-operands pass would incorrectly fold immediate values into COPY instructions targeting av_32 registers. The pass now checks register class constraints before attempting to fold the immediate.
…n folding immediate into copies (llvm#131387) Fixes llvm#130020 This fixes an issue where the si-fold-operands pass would incorrectly fold immediate values into COPY instructions targeting av_32 registers. The pass now checks register class constraints before attempting to fold the immediate.
Fixes #130020
This fixes an issue where the si-fold-operands pass would incorrectly fold immediate values into COPY instructions targeting av_32 registers.
The pass now checks register class constraints before attempting to fold the immediate.