Skip to content

[AMDGPU][MC] Don't crash on decoding invalid SOP1 ssrc0 operands. #130302

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
Mar 8, 2025

Conversation

kosarev
Copy link
Collaborator

@kosarev kosarev commented Mar 7, 2025

These are encoded as 8-bit fields.

@kosarev kosarev requested review from jayfoad, arsenm and mbrkusanin March 7, 2025 16:17
@llvmbot llvmbot added backend:AMDGPU llvm:mc Machine (object) code labels Mar 7, 2025
@llvmbot
Copy link
Member

llvmbot commented Mar 7, 2025

@llvm/pr-subscribers-mc

@llvm/pr-subscribers-backend-amdgpu

Author: Ivan Kosarev (kosarev)

Changes

These are encoded as 8-bit fields.


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

3 Files Affected:

  • (modified) llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp (+19-15)
  • (modified) llvm/test/MC/Disassembler/AMDGPU/gfx11_dasm_sop1.txt (+3)
  • (modified) llvm/test/MC/Disassembler/AMDGPU/gfx12_dasm_sop1.txt (+3)
diff --git a/llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp b/llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
index afed8b999d8eb..728ce125eba2d 100644
--- a/llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
+++ b/llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
@@ -176,9 +176,12 @@ static DecodeStatus decodeSrcOp(MCInst &Inst, unsigned EncSize,
 
 // Decoder for registers. Imm(7-bit) is number of register, uses decodeSrcOp to
 // get register class. Used by SGPR only operands.
-#define DECODE_OPERAND_REG_7(RegClass, OpWidth)                                \
+#define DECODE_OPERAND_SREG_7(RegClass, OpWidth)                               \
   DECODE_SrcOp(Decode##RegClass##RegisterClass, 7, OpWidth, Imm, false, 0)
 
+#define DECODE_OPERAND_SREG_8(RegClass, OpWidth)                               \
+  DECODE_SrcOp(Decode##RegClass##RegisterClass, 8, OpWidth, Imm, false, 0)
+
 // Decoder for registers. Imm(10-bit): Imm{7-0} is number of register,
 // Imm{9} is acc(agpr or vgpr) Imm{8} should be 0 (see VOP3Pe_SMFMAC).
 // Set Imm{8} to 1 (IS_VGPR) to decode using 'enum10' from decodeSrcOp.
@@ -270,20 +273,21 @@ DECODE_OPERAND_REG_8(VReg_384)
 DECODE_OPERAND_REG_8(VReg_512)
 DECODE_OPERAND_REG_8(VReg_1024)
 
-DECODE_OPERAND_REG_7(SReg_32, OPW32)
-DECODE_OPERAND_REG_7(SReg_32_XM0, OPW32)
-DECODE_OPERAND_REG_7(SReg_32_XEXEC, OPW32)
-DECODE_OPERAND_REG_7(SReg_32_XM0_XEXEC, OPW32)
-DECODE_OPERAND_REG_7(SReg_32_XEXEC_HI, OPW32)
-DECODE_OPERAND_REG_7(SReg_64, OPW64)
-DECODE_OPERAND_REG_7(SReg_64_XEXEC, OPW64)
-DECODE_OPERAND_REG_7(SReg_64_XEXEC_XNULL, OPW64)
-DECODE_OPERAND_REG_7(SReg_96, OPW96)
-DECODE_OPERAND_REG_7(SReg_128, OPW128)
-DECODE_OPERAND_REG_7(SReg_128_XNULL, OPW128)
-DECODE_OPERAND_REG_7(SReg_256, OPW256)
-DECODE_OPERAND_REG_7(SReg_256_XNULL, OPW256)
-DECODE_OPERAND_REG_7(SReg_512, OPW512)
+DECODE_OPERAND_SREG_7(SReg_32, OPW32)
+DECODE_OPERAND_SREG_7(SReg_32_XM0, OPW32)
+DECODE_OPERAND_SREG_7(SReg_32_XEXEC, OPW32)
+DECODE_OPERAND_SREG_7(SReg_32_XM0_XEXEC, OPW32)
+DECODE_OPERAND_SREG_7(SReg_32_XEXEC_HI, OPW32)
+DECODE_OPERAND_SREG_7(SReg_64_XEXEC, OPW64)
+DECODE_OPERAND_SREG_7(SReg_64_XEXEC_XNULL, OPW64)
+DECODE_OPERAND_SREG_7(SReg_96, OPW96)
+DECODE_OPERAND_SREG_7(SReg_128, OPW128)
+DECODE_OPERAND_SREG_7(SReg_128_XNULL, OPW128)
+DECODE_OPERAND_SREG_7(SReg_256, OPW256)
+DECODE_OPERAND_SREG_7(SReg_256_XNULL, OPW256)
+DECODE_OPERAND_SREG_7(SReg_512, OPW512)
+
+DECODE_OPERAND_SREG_8(SReg_64, OPW64)
 
 DECODE_OPERAND_REG_8(AGPR_32)
 DECODE_OPERAND_REG_8(AReg_64)
diff --git a/llvm/test/MC/Disassembler/AMDGPU/gfx11_dasm_sop1.txt b/llvm/test/MC/Disassembler/AMDGPU/gfx11_dasm_sop1.txt
index 291f348a35cc6..99a210b4ed088 100644
--- a/llvm/test/MC/Disassembler/AMDGPU/gfx11_dasm_sop1.txt
+++ b/llvm/test/MC/Disassembler/AMDGPU/gfx11_dasm_sop1.txt
@@ -2548,6 +2548,9 @@
 # GFX11: s_setpc_b64 vcc                         ; encoding: [0x6a,0x48,0x80,0xbe]
 0x6a,0x48,0x80,0xbe
 
+# GFX11: s_setpc_b64 -11/*Invalid immediate*/    ; encoding: [0xf5,0x48,0x80,0xbe]
+0xcb,0x48,0xf5,0xbe
+
 # GFX11: s_sext_i32_i16 exec_hi, s1              ; encoding: [0x01,0x0f,0xff,0xbe]
 0x01,0x0f,0xff,0xbe
 
diff --git a/llvm/test/MC/Disassembler/AMDGPU/gfx12_dasm_sop1.txt b/llvm/test/MC/Disassembler/AMDGPU/gfx12_dasm_sop1.txt
index fa7d020bdd726..90528a97fa1df 100644
--- a/llvm/test/MC/Disassembler/AMDGPU/gfx12_dasm_sop1.txt
+++ b/llvm/test/MC/Disassembler/AMDGPU/gfx12_dasm_sop1.txt
@@ -3267,6 +3267,9 @@
 # GFX12: s_setpc_b64 vcc                         ; encoding: [0x6a,0x48,0x80,0xbe]
 0x6a,0x48,0x80,0xbe
 
+# GFX12: s_setpc_b64 -11/*Invalid immediate*/    ; encoding: [0xf5,0x48,0x80,0xbe]
+0xcb,0x48,0xf5,0xbe
+
 # GFX12: s_sext_i32_i16 exec_hi, s1              ; encoding: [0x01,0x0f,0xff,0xbe]
 0x01,0x0f,0xff,0xbe
 

@@ -2548,6 +2548,9 @@
# GFX11: s_setpc_b64 vcc ; encoding: [0x6a,0x48,0x80,0xbe]
0x6a,0x48,0x80,0xbe

# GFX11: s_setpc_b64 -11/*Invalid immediate*/ ; encoding: [0xf5,0x48,0x80,0xbe]
0xcb,0x48,0xf5,0xbe
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe test with more operand bit widths? Or I guess only SReg_64 changed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it's just SReg_64. I hope to have to time to check other cases more thoroughly, but we need this s_setpc fix as a matter of urgency.

@kosarev kosarev merged commit 15869a8 into llvm:main Mar 8, 2025
12 of 14 checks passed
@kosarev kosarev deleted the sreg64-8 branch March 8, 2025 01:10
@llvm-ci
Copy link
Collaborator

llvm-ci commented Mar 8, 2025

LLVM Buildbot has detected a new failure on builder openmp-offload-amdgpu-runtime-2 running on rocm-worker-hw-02 while building llvm at step 8 "Add check check-llvm".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/10/builds/868

Here is the relevant piece of the build log for the reference
Step 8 (Add check check-llvm) failure: test (failure)
******************** TEST 'LLVM-Unit :: Support/./SupportTests/135/175' FAILED ********************
Script(shard):
--
GTEST_OUTPUT=json:/home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.build/unittests/Support/./SupportTests-LLVM-Unit-1542977-135-175.json GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=175 GTEST_SHARD_INDEX=135 /home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.build/unittests/Support/./SupportTests
--

Script:
--
/home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.build/unittests/Support/./SupportTests --gtest_filter=Caching.WriteAfterCommit
--
/home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.src/llvm/unittests/Support/Caching.cpp:110: Failure
Value of: llvm::detail::TakeError(CFS->commit())
Expected: succeeded
  Actual: failed  (Failed to rename temporary file /tmp/lit-tmp-uog0grm2/llvm_test_cache/LLVMTest-fb811a.tmp.o to /tmp/lit-tmp-uog0grm2/llvm_test_cache/llvmcache-foo: No such file or directory
)


/home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.src/llvm/unittests/Support/Caching.cpp:110
Value of: llvm::detail::TakeError(CFS->commit())
Expected: succeeded
  Actual: failed  (Failed to rename temporary file /tmp/lit-tmp-uog0grm2/llvm_test_cache/LLVMTest-fb811a.tmp.o to /tmp/lit-tmp-uog0grm2/llvm_test_cache/llvmcache-foo: No such file or directory
)



********************


@llvm-ci
Copy link
Collaborator

llvm-ci commented Mar 8, 2025

LLVM Buildbot has detected a new failure on builder lld-x86_64-ubuntu-fast running on as-builder-4 while building llvm at step 6 "test-build-unified-tree-check-all".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/33/builds/12645

Here is the relevant piece of the build log for the reference
Step 6 (test-build-unified-tree-check-all) failure: test (failure)
******************** TEST 'LLVM-Unit :: Support/./SupportTests/46/88' FAILED ********************
Script(shard):
--
GTEST_OUTPUT=json:/home/buildbot/worker/as-builder-4/ramdisk/lld-x86_64/build/unittests/Support/./SupportTests-LLVM-Unit-588914-46-88.json GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=88 GTEST_SHARD_INDEX=46 /home/buildbot/worker/as-builder-4/ramdisk/lld-x86_64/build/unittests/Support/./SupportTests
--

Script:
--
/home/buildbot/worker/as-builder-4/ramdisk/lld-x86_64/build/unittests/Support/./SupportTests --gtest_filter=Caching.Normal
--
/home/buildbot/worker/as-builder-4/ramdisk/lld-x86_64/llvm-project/llvm/unittests/Support/Caching.cpp:62: Failure
Value of: llvm::detail::TakeError(CFS->commit())
Expected: succeeded
  Actual: failed  (Failed to rename temporary file /home/buildbot/worker/as-builder-4/ramdisk/lld-x86_64/build/lit-tmp-_qjhr994/llvm_test_cache/LLVMTest-b154fd.tmp.o to /home/buildbot/worker/as-builder-4/ramdisk/lld-x86_64/build/lit-tmp-_qjhr994/llvm_test_cache/llvmcache-foo: No such file or directory
)


/home/buildbot/worker/as-builder-4/ramdisk/lld-x86_64/llvm-project/llvm/unittests/Support/Caching.cpp:62
Value of: llvm::detail::TakeError(CFS->commit())
Expected: succeeded
  Actual: failed  (Failed to rename temporary file /home/buildbot/worker/as-builder-4/ramdisk/lld-x86_64/build/lit-tmp-_qjhr994/llvm_test_cache/LLVMTest-b154fd.tmp.o to /home/buildbot/worker/as-builder-4/ramdisk/lld-x86_64/build/lit-tmp-_qjhr994/llvm_test_cache/llvmcache-foo: No such file or directory
)



********************


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:AMDGPU llvm:mc Machine (object) code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants