This is an archive of the discontinued LLVM Phabricator instance.

Differential D125907

[AMDGPU][MC][GFX8+] Correct SMEM offset parsing
ClosedPublic

Authored by dp on May 18 2022, 10:00 AM.

Details

Reviewers
rampitec
foad
kosarev
Commits
rGf598dfb3bf8c: [AMDGPU][MC][GFX8+] Correct SMEM offset parsing
Summary

See bug 55563.

Diff Detail

Unit TestsFailed

TimeTest
60,250 msx64 debian > AddressSanitizer-x86_64-linux-dynamic.TestCases::scariness_score_test.cpp
60,110 msx64 debian > AddressSanitizer-x86_64-linux.TestCases::scariness_score_test.cpp
60,050 msx64 debian > MLIR.Examples/standalone::test.toy
60,030 msx64 debian > ThreadSanitizer-x86_64.ThreadSanitizer-x86_64::signal_sync.cpp
60,020 msx64 debian > libFuzzer.libFuzzer::fuzzer-leak.test
View Full Test Results (7 Failed)

Event Timeline

dp created this revision.May 18 2022, 10:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 18 2022, 10:00 AM
Herald added subscribers: kosarev, jsilvanus, hsmhsm and 10 others. · View Herald Transcript
dp requested review of this revision.May 18 2022, 10:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 18 2022, 10:00 AM
Herald added subscribers: llvm-commits, wdng. · View Herald Transcript
rampitec accepted this revision.May 18 2022, 10:20 AM
This revision is now accepted and ready to land.May 18 2022, 10:20 AM
Harbormaster completed remote builds in B165160: Diff 430421.May 18 2022, 11:03 AM
kosarev added inline comments.May 19 2022, 1:05 AM
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
7733

Can we just say something like isImmTy(ImmTyNone) || isImmTy(ImmTyOffset)? The way it is, it's not clear what's so special about cache policies in relation with SMEM offsets.

dp updated this revision to Diff 430628.May 19 2022, 3:35 AM
dp marked an inline comment as done.
dp added a reviewer: kosarev.

Updated as suggested by Ivan.

llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
7733

Good point, thanks!

kosarev accepted this revision.May 19 2022, 3:53 AM

LGTM.

Harbormaster completed remote builds in B165294: Diff 430628.May 19 2022, 5:05 AM
Closed by commit rGf598dfb3bf8c: [AMDGPU][MC][GFX8+] Correct SMEM offset parsing (authored by dp). · Explain WhyMay 20 2022, 4:01 AM
This revision was automatically updated to reflect the committed changes.
dp added a commit: rGf598dfb3bf8c: [AMDGPU][MC][GFX8+] Correct SMEM offset parsing.

Revision Contents

PathSize
llvm/
lib/
Target/
AMDGPU/
AsmParser/
2 lines
test/
MC/
AMDGPU/
6 lines
3 lines
3 lines

Diff 430421

llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp

  • This file is larger than 256 KB, so syntax highlighting is disabled by default.
Show First 20 Lines • Show All 7,724 Lines • ▼ Show 20 Lines
// smrd // smrd
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
bool AMDGPUOperand::isSMRDOffset8() const { bool AMDGPUOperand::isSMRDOffset8() const {
return isImm() && isUInt<8>(getImm()); return isImm() && isUInt<8>(getImm());
} }
bool AMDGPUOperand::isSMEMOffset() const { bool AMDGPUOperand::isSMEMOffset() const {
return isImm(); // Offset range is checked later by validator. return isImm() && !isCPol(); // Offset range is checked later by validator.
kosarevUnsubmitted
Done
ReplyInline Actions

Can we just say something like isImmTy(ImmTyNone) || isImmTy(ImmTyOffset)? The way it is, it's not clear what's so special about cache policies in relation with SMEM offsets.

kosarev: Can we just say something like `isImmTy(ImmTyNone) || isImmTy(ImmTyOffset)`? The way it is…
dpAuthorUnsubmitted
Done
ReplyInline Actions

Good point, thanks!

dp: Good point, thanks!
} }
bool AMDGPUOperand::isSMRDLiteralOffset() const { bool AMDGPUOperand::isSMRDLiteralOffset() const {
// 32-bit literals are only supported on CI and we only want to use them // 32-bit literals are only supported on CI and we only want to use them
// when the offset is > 8-bits. // when the offset is > 8-bits.
return isImm() && !isUInt<8>(getImm()) && isUInt<32>(getImm()); return isImm() && !isUInt<8>(getImm()) && isUInt<32>(getImm());
} }
▲ Show 20 Lines • Show All 1,100 Lines • Show Last 20 Lines

llvm/test/MC/AMDGPU/gfx10_asm_smem.s

Show First 20 Lines • Show All 1,336 Lines • ▼ Show 20 Lines
s_buffer_atomic_umax s5, s[4:7], s2 dlc s_buffer_atomic_umax s5, s[4:7], s2 dlc
// GFX10: encoding: [0x42,0x41,0x1c,0xf5,0x00,0x00,0x00,0x04] // GFX10: encoding: [0x42,0x41,0x1c,0xf5,0x00,0x00,0x00,0x04]
s_buffer_atomic_umin s5, s[4:7], s2 dlc s_buffer_atomic_umin s5, s[4:7], s2 dlc
// GFX10: encoding: [0x42,0x41,0x14,0xf5,0x00,0x00,0x00,0x04] // GFX10: encoding: [0x42,0x41,0x14,0xf5,0x00,0x00,0x00,0x04]
s_buffer_atomic_xor s5, s[4:7], s2 dlc s_buffer_atomic_xor s5, s[4:7], s2 dlc
// GFX10: encoding: [0x42,0x41,0x28,0xf5,0x00,0x00,0x00,0x04] // GFX10: encoding: [0x42,0x41,0x28,0xf5,0x00,0x00,0x00,0x04]
s_load_dword s1, s[2:3] glc
// GFX10: s_load_dword s1, s[2:3], 0x0 glc ; encoding: [0x41,0x00,0x01,0xf4,0x00,0x00,0x00,0xfa]
s_load_dword s1, s[2:3] dlc
// GFX10: s_load_dword s1, s[2:3], 0x0 dlc ; encoding: [0x41,0x40,0x00,0xf4,0x00,0x00,0x00,0xfa]

llvm/test/MC/AMDGPU/gfx8_asm_smem.s

Show First 20 Lines • Show All 1,157 Lines • ▼ Show 20 Lines
s_atc_probe_buffer 0x0, s[8:11], ttmp11 s_atc_probe_buffer 0x0, s[8:11], ttmp11
// CHECK: [0x04,0x00,0x9c,0xc0,0x7b,0x00,0x00,0x00] // CHECK: [0x04,0x00,0x9c,0xc0,0x7b,0x00,0x00,0x00]
s_atc_probe_buffer 0x0, s[8:11], m0 s_atc_probe_buffer 0x0, s[8:11], m0
// CHECK: [0x04,0x00,0x9c,0xc0,0x7c,0x00,0x00,0x00] // CHECK: [0x04,0x00,0x9c,0xc0,0x7c,0x00,0x00,0x00]
s_atc_probe_buffer 0x0, s[8:11], 0x7ffff s_atc_probe_buffer 0x0, s[8:11], 0x7ffff
// CHECK: [0x04,0x00,0x9e,0xc0,0xff,0xff,0x07,0x00] // CHECK: [0x04,0x00,0x9e,0xc0,0xff,0xff,0x07,0x00]
s_load_dword s1, s[2:3] glc
// CHECK: s_load_dword s1, s[2:3], 0x0 glc ; encoding: [0x41,0x00,0x03,0xc0,0x00,0x00,0x00,0x00]

llvm/test/MC/AMDGPU/gfx9_asm_smem.s

Show First 20 Lines • Show All 4,469 Lines • ▼ Show 20 Lines
s_atomic_dec_x2 s[10:11], s[2:3], m0 s_atomic_dec_x2 s[10:11], s[2:3], m0
// CHECK: [0x81,0x02,0xb0,0xc2,0x7c,0x00,0x00,0x00] // CHECK: [0x81,0x02,0xb0,0xc2,0x7c,0x00,0x00,0x00]
s_atomic_dec_x2 s[10:11], s[2:3], 0x0 s_atomic_dec_x2 s[10:11], s[2:3], 0x0
// CHECK: [0x81,0x02,0xb2,0xc2,0x00,0x00,0x00,0x00] // CHECK: [0x81,0x02,0xb2,0xc2,0x00,0x00,0x00,0x00]
s_atomic_dec_x2 s[10:11], s[2:3], s0 glc s_atomic_dec_x2 s[10:11], s[2:3], s0 glc
// CHECK: [0x81,0x02,0xb1,0xc2,0x00,0x00,0x00,0x00] // CHECK: [0x81,0x02,0xb1,0xc2,0x00,0x00,0x00,0x00]
s_load_dword s1, s[2:3] glc
// CHECK: s_load_dword s1, s[2:3], 0x0 glc ; encoding: [0x41,0x00,0x03,0xc0,0x00,0x00,0x00,0x00]