[AMDGPU] Relax SOPK immediate signed/unsigned check #77015
Conversation
Machine verifier complains about an invalid SOPK immediate based on it signedness. It does not seem we have a good sanity in the SOPK definitions. For instance there is no reason for S_SETREG_B32 immedaite to be defined as SIMM16. A useful value 63489 cannot be used and we force users to use -2047 instead which makes a little sense, not obvious, and on top of that only enforced in the debug compiler build. At the same time SP3 itself does not have this restriction and accepts both signed and unsigned value, so we also have SP3 compatibility issue. Ralax the verifier and allow both signed and unsigned constants.
|
@llvm/pr-subscribers-backend-amdgpu Author: Stanislav Mekhanoshin (rampitec) ChangesMachine verifier complains about an invalid SOPK immediate based on it signedness. It does not seem we have a good sanity in the SOPK definitions. For instance there is no reason for S_SETREG_B32 immedaite to be defined as SIMM16. A useful value 63489 cannot be used and we force users to use -2047 instead which makes a little sense, not obvious, and on top of that only enforced in the debug compiler build. At the same time SP3 itself does not have this restriction and accepts both signed and unsigned value, so we also have SP3 compatibility issue. Ralax the verifier and allow both signed and unsigned constants. Full diff: https://github.com/llvm/llvm-project/pull/77015.diff 2 Files Affected:
diff --git a/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp b/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
index 67992929ab3567..0dc2806fa30264 100644
--- a/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
+++ b/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
@@ -4891,16 +4891,9 @@ bool SIInstrInfo::verifyInstruction(const MachineInstr &MI,
}
} else {
uint64_t Imm = Op->getImm();
- if (sopkIsZext(MI)) {
- if (!isUInt<16>(Imm)) {
- ErrInfo = "invalid immediate for SOPK instruction";
- return false;
- }
- } else {
- if (!isInt<16>(Imm)) {
- ErrInfo = "invalid immediate for SOPK instruction";
- return false;
- }
+ if (!isUInt<16>(Imm) && !isInt<16>(Imm)) {
+ ErrInfo = "invalid immediate for SOPK instruction";
+ return false;
}
}
}
diff --git a/llvm/test/CodeGen/AMDGPU/llvm.amdgcn.s.setreg.ll b/llvm/test/CodeGen/AMDGPU/llvm.amdgcn.s.setreg.ll
index d2c14f2401fc35..b2e987ae37fd93 100644
--- a/llvm/test/CodeGen/AMDGPU/llvm.amdgcn.s.setreg.ll
+++ b/llvm/test/CodeGen/AMDGPU/llvm.amdgcn.s.setreg.ll
@@ -1433,6 +1433,39 @@ define amdgpu_kernel void @test_setreg_set_4_bits_straddles_round_and_denorm() {
ret void
}
+define amdgpu_ps void @test_63489(i32 inreg %var.mode) {
+; GFX6-LABEL: test_63489:
+; GFX6: ; %bb.0:
+; GFX6-NEXT: s_setreg_b32 hwreg(HW_REG_MODE), s0 ; encoding: [0x01,0xf8,0x80,0xb9]
+; GFX6-NEXT: ;;#ASMSTART
+; GFX6-NEXT: ;;#ASMEND
+; GFX6-NEXT: s_endpgm ; encoding: [0x00,0x00,0x81,0xbf]
+;
+; GFX789-LABEL: test_63489:
+; GFX789: ; %bb.0:
+; GFX789-NEXT: s_setreg_b32 hwreg(HW_REG_MODE), s0 ; encoding: [0x01,0xf8,0x00,0xb9]
+; GFX789-NEXT: ;;#ASMSTART
+; GFX789-NEXT: ;;#ASMEND
+; GFX789-NEXT: s_endpgm ; encoding: [0x00,0x00,0x81,0xbf]
+;
+; GFX10-LABEL: test_63489:
+; GFX10: ; %bb.0:
+; GFX10-NEXT: s_setreg_b32 hwreg(HW_REG_MODE), s0 ; encoding: [0x01,0xf8,0x80,0xb9]
+; GFX10-NEXT: ;;#ASMSTART
+; GFX10-NEXT: ;;#ASMEND
+; GFX10-NEXT: s_endpgm ; encoding: [0x00,0x00,0x81,0xbf]
+;
+; GFX11-LABEL: test_63489:
+; GFX11: ; %bb.0:
+; GFX11-NEXT: s_setreg_b32 hwreg(HW_REG_MODE), s0 ; encoding: [0x01,0xf8,0x00,0xb9]
+; GFX11-NEXT: ;;#ASMSTART
+; GFX11-NEXT: ;;#ASMEND
+; GFX11-NEXT: s_endpgm ; encoding: [0x00,0x00,0xb0,0xbf]
+ call void @llvm.amdgcn.s.setreg(i32 63489, i32 %var.mode)
+ call void asm sideeffect "", ""()
+ ret void
+}
+
; FIXME: Broken for DAG
; define void @test_setreg_roundingmode_var_vgpr(i32 %var.mode) {
; call void @llvm.amdgcn.s.setreg(i32 4097, i32 %var.mode)
|
The problem with the check is that it triggers on user code, not something we are creating inside the compiler. A user code just calls setreg intrinsic and fall into the problem. What is worse it only happens with debug build. I could change the operand to be unsigned, although it is not what is written in SP3, but it will break sources which are now using negative constants to w/a this problem. |
|
Ping |
You can use an SDNodeXForm on the immediate to fix it up during selection |
|
Machine verifier complains about an invalid SOPK immediate based on it signedness. It does not seem we have a good sanity in the SOPK definitions. For instance there is no reason for S_SETREG_B32 immedaite to be defined as SIMM16. A useful value 63489 cannot be used and we force users to use -2047 instead which makes a little sense, not obvious, and on top of that only enforced in the debug compiler build. At the same time SP3 itself does not have this restriction and accepts both signed and unsigned value, so we also have SP3 compatibility issue.
Ralax the verifier and allow both signed and unsigned constants.