Skip to content

Commit 944d977

Browse files
committed
[AMDGPU] comgr: rewrite s_sethalt to s_nop for GFX1250 A0
Rewrites every s_sethalt to s_nop 0 in-place (4-byte overwrite, no length change, no offset shifts). Defensive: LLVM almost never emits s_sethalt in production code (only via the int_amdgcn_s_sethalt intrinsic, mostly in debug builds), so this pass is dormant on most inputs -- it exists for defense-in-depth against any input ELF, compiler-emitted or hand-written, that ships s_sethalt. The replacement bytes come from LS.SNopBytes (pre-encoded at initLLVM() time, the same source the splitter uses for trampoline padding). Wired into b0a0.cpp dispatcher AFTER applyVop3pxWrapPatch (the order doesn't matter functionally -- sethalt-fix and the wrap pass don't interact -- but keeping all the A0-only post-loop passes adjacent makes the dispatch order easy to read). LIT test (hotswap-sethalt-fix.s): 4 cases -- bare s_sethalt is neutralized, s_sethalt before VOP3PX2 is neutralized, multiple s_sethalt instances are all neutralized, kernels without s_sethalt are not modified.
1 parent e4812cd commit 944d977

6 files changed

Lines changed: 214 additions & 0 deletions

File tree

amd/comgr/CMakeLists.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,7 @@ set(SOURCES
108108
src/comgr-hotswap-llvm.cpp
109109
src/comgr-hotswap-patch-f32-to-e5m3.cpp
110110
src/comgr-hotswap-patch-inplace.cpp
111+
src/comgr-hotswap-patch-sethalt-fix.cpp
111112
src/comgr-hotswap-patch-vop3px2-src2.cpp
112113
src/comgr-hotswap-patch-wmma-hazard.cpp
113114
src/comgr-hotswap-patch-wmma-scale16.cpp

amd/comgr/src/comgr-hotswap-b0a0.cpp

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -404,6 +404,8 @@ applyGfx1250B0toA0Rules(std::vector<InternalDecodedInst> &Decoded,
404404
Patched += VT.applyWmmaHazardPatch(Ctx);
405405
if (VT.applyVop3px2Src2Fix)
406406
Patched += VT.applyVop3px2Src2Fix(Ctx);
407+
if (VT.applySethaltFixPatch)
408+
Patched += VT.applySethaltFixPatch(Ctx);
407409

408410
for (const llvm::StringMapEntry<KernelPatchStats> &KV : KernelStats) {
409411
StringRef KName = KV.first();

amd/comgr/src/comgr-hotswap-internal.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -601,6 +601,7 @@ struct HotswapPatchVTable {
601601
// loop completes.
602602
uint32_t (*applyWmmaHazardPatch)(PatchContext &) = nullptr;
603603
uint32_t (*applyVop3px2Src2Fix)(PatchContext &) = nullptr;
604+
uint32_t (*applySethaltFixPatch)(PatchContext &) = nullptr;
604605
};
605606

606607
/// Walk comgr-hotswap-patches.def and bind every patch module's
Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,111 @@
1+
//===- comgr-hotswap-patch-sethalt-fix.cpp - in-shader sethalt fix ------===//
2+
//
3+
// Part of Comgr, under the Apache License v2.0 with LLVM Exceptions. See
4+
// amd/comgr/LICENSE.TXT in this repository for license information.
5+
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
6+
//
7+
//===----------------------------------------------------------------------===//
8+
///
9+
/// \file
10+
/// Strong-symbol override for applySethaltFixPatch. Defensive hotswap
11+
/// pass for the GFX1250 A0 LD_SCALE+WMMA clause-break bug as triggered
12+
/// by the in-shader `s_sethalt` instruction.
13+
///
14+
/// On GFX1250 A0, the SQ has an FSM bug where a wave halt issued just
15+
/// before LD_SCALE breaks the implicit LD_SCALE+WMMA clause: an
16+
/// asynchronous clause-break event during the halt allows other waves
17+
/// to interleave between this wave's LD_SCALE and its WMMA, which
18+
/// either picks up a leaked scale (corrupting another wave's WMMA) or
19+
/// loses its own scale (this wave's WMMA runs unscaled).
20+
///
21+
/// "Software running on A0 cannot use SQ_CMD.HALT operations" covers
22+
/// two halt sources:
23+
/// 1. `s_sethalt` -- in-shader instruction. Reachable from compiler
24+
/// code objects; this pass handles it.
25+
/// 2. SQ_CMD.HALT (SETHALT) -- external register write from the host
26+
/// (debugger breakpoints, rocprofiler PC sampling, kernel CWSR
27+
/// paths). NOT reachable from COMGR; needs ROCdbgapi /
28+
/// rocprofiler / runtime changes to skip on GFX1250 A0. Out of
29+
/// scope for this pass.
30+
///
31+
/// LLVM almost never emits `s_sethalt` in production code -- it appears
32+
/// only via the `int_amdgcn_s_sethalt` intrinsic, which is largely
33+
/// confined to debug builds. This pass exists for defense-in-depth
34+
/// against any input ELF (compiler-emitted or hand-written) that
35+
/// contains the instruction, and would otherwise hit the SQ FSM bug
36+
/// when the kernel also uses VOP3PX2.
37+
///
38+
/// Strategy: detect every `s_sethalt` (mnemonic match against the MC
39+
/// printer output) and rewrite to `s_nop 0` in-place. Same 4 bytes, no
40+
/// relocation, no length change. Replacement bytes come from
41+
/// LS.SNopBytes (pre-encoded at initLLVM() time -- same source the
42+
/// splitter uses for trampoline padding). The shader proceeds without
43+
/// the halt; if the halt was for an in-shader debug breakpoint, that's
44+
/// the correct trade-off on A0 (the debugger should switch to
45+
/// trap-based breakpoints, which is the source-2 fix the debugger team
46+
/// owns).
47+
///
48+
//===----------------------------------------------------------------------===//
49+
50+
#include "comgr-hotswap-internal.h"
51+
52+
#include "llvm/ADT/StringExtras.h"
53+
#include "llvm/ADT/StringRef.h"
54+
55+
using namespace llvm;
56+
57+
namespace COMGR {
58+
namespace hotswap {
59+
namespace {
60+
61+
constexpr StringLiteral SSethaltMnemonic = "s_sethalt";
62+
63+
uint32_t applySethaltFixPatchImpl(PatchContext &Ctx) {
64+
if (Ctx.LS.SNopBytes.size() != MinInstSize) {
65+
log() << "hotswap: error: sethalt-fix: LS.SNopBytes is not " << MinInstSize
66+
<< " bytes (got " << Ctx.LS.SNopBytes.size() << ")\n";
67+
return 0;
68+
}
69+
70+
uint32_t Patched = 0;
71+
for (const InternalDecodedInst &DI : Ctx.Decoded) {
72+
if (DI.Mnemonic != SSethaltMnemonic)
73+
continue;
74+
if (DI.Size != MinInstSize) {
75+
log() << "hotswap: error: sethalt-fix: s_sethalt at .text offset 0x"
76+
<< utohexstr(DI.Offset) << " has unexpected size " << DI.Size
77+
<< " (expected " << MinInstSize << ")\n";
78+
continue;
79+
}
80+
// DI came from decodeTextSection over Ctx.Text, so this should only fire
81+
// if a future caller supplies an inconsistent decoded stream/text pair.
82+
if (DI.Offset + DI.Size > Ctx.TextSize) {
83+
log() << "hotswap: error: sethalt-fix: s_sethalt at .text offset 0x"
84+
<< utohexstr(DI.Offset) << " extends past .text size 0x"
85+
<< utohexstr(Ctx.TextSize) << "\n";
86+
continue;
87+
}
88+
89+
std::memcpy(Ctx.Text + DI.Offset, Ctx.LS.SNopBytes.data(), MinInstSize);
90+
91+
std::string KernelName =
92+
Ctx.Elf.findKernelAtOffset(DI.Offset + Ctx.Elf.textAddr());
93+
StringRef KernelDisplay =
94+
KernelName.empty() ? StringRef("<unknown>") : StringRef(KernelName);
95+
log() << "hotswap: sethalt-fix: neutralized s_sethalt in kernel '"
96+
<< KernelDisplay << "' at .text offset 0x" << utohexstr(DI.Offset)
97+
<< "\n";
98+
++Patched;
99+
}
100+
101+
return Patched;
102+
}
103+
104+
} // namespace
105+
106+
void registerSethaltFixPatch(HotswapPatchVTable &VT) {
107+
VT.applySethaltFixPatch = &applySethaltFixPatchImpl;
108+
}
109+
110+
} // namespace hotswap
111+
} // namespace COMGR

amd/comgr/src/comgr-hotswap-patches.def

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929

3030
HOTSWAP_PATCH(InPlace)
3131
HOTSWAP_PATCH(Scratch)
32+
HOTSWAP_PATCH(SethaltFix)
3233
HOTSWAP_PATCH(Trampoline)
3334
HOTSWAP_PATCH(Vop3px2Src2)
3435
HOTSWAP_PATCH(WmmaHazard)
Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
// Test sethalt-fix patches for GFX1250 A0 LD_SCALE+WMMA clause-break
2+
// triggered by in-shader s_sethalt (FPV ticket: sethalt before
3+
// LD_SCALE breaks the implicit clause).
4+
//
5+
// The hotswap sethalt-fix pass detects every `s_sethalt` instruction
6+
// in the input ELF and replaces it with `s_nop 0` in-place. The
7+
// shader proceeds without the halt; if the halt was for an in-shader
8+
// debug breakpoint, that's the correct trade-off on A0 (debugger
9+
// should switch to trap-based breakpoints, which lives outside the
10+
// code object).
11+
//
12+
// RUN: %clang -target amdgcn-amd-amdhsa -mcpu=gfx1250 -nostdlib %s -o %t.elf
13+
// RUN: hotswap-rewrite %t.elf \
14+
// RUN: amdgcn-amd-amdhsa--gfx1250 amdgcn-amd-amdhsa--gfx1250 \
15+
// RUN: --output %t.out.elf \
16+
// RUN: | %FileCheck --check-prefix=API %s
17+
// API: RESULT: SUCCESS
18+
// RUN: %llvm-objdump -d %t.out.elf | %FileCheck %s
19+
20+
.amdgcn_target "amdgcn-amd-amdhsa--gfx1250"
21+
22+
// -- Test 1: bare s_sethalt is neutralized to s_nop -------------------------
23+
//
24+
// The single s_sethalt in this kernel must disappear; an s_nop must
25+
// appear in its place. No other instructions are affected.
26+
//
27+
// CHECK-LABEL: <test_bare_sethalt>:
28+
// CHECK-NOT: s_sethalt
29+
// CHECK: s_nop
30+
// CHECK: s_endpgm
31+
.globl test_bare_sethalt
32+
.p2align 8
33+
.type test_bare_sethalt,@function
34+
test_bare_sethalt:
35+
s_sethalt 1
36+
s_endpgm
37+
.size test_bare_sethalt, .-test_bare_sethalt
38+
39+
// -- Test 2: s_sethalt before VOP3PX2 (the FPV scenario) --------------------
40+
//
41+
// This is the exact FPV-ticket repro shape: s_sethalt halts the wave
42+
// just before a v_wmma_scale_* (VOP3PX2) instruction would issue.
43+
// On A0, the halt would break the implicit LD_SCALE+WMMA clause and
44+
// allow scale-factor leakage. After hotswap, the s_sethalt is gone
45+
// and the VOP3PX2 runs without the halt-induced race.
46+
//
47+
// CHECK-LABEL: <test_sethalt_before_vop3px2>:
48+
// CHECK-NOT: s_sethalt
49+
// CHECK: s_nop
50+
// CHECK: v_wmma_scale_f32_16x16x128_f8f6f4
51+
// CHECK: s_endpgm
52+
.globl test_sethalt_before_vop3px2
53+
.p2align 8
54+
.type test_sethalt_before_vop3px2,@function
55+
test_sethalt_before_vop3px2:
56+
s_sethalt 1
57+
v_wmma_scale_f32_16x16x128_f8f6f4 v[16:23], v[0:15], v[8:23], v[16:23], 0, 0
58+
s_endpgm
59+
.size test_sethalt_before_vop3px2, .-test_sethalt_before_vop3px2
60+
61+
// -- Test 3: multiple s_sethalt instances all neutralized -------------------
62+
//
63+
// The pass walks decoded[] linearly, so all s_sethalt occurrences
64+
// should be neutralized regardless of position or simm operand.
65+
//
66+
// CHECK-LABEL: <test_multiple_sethalt>:
67+
// CHECK-NOT: s_sethalt
68+
// CHECK: s_nop
69+
// CHECK: v_add_f32
70+
// CHECK: s_nop
71+
// CHECK: s_endpgm
72+
.globl test_multiple_sethalt
73+
.p2align 8
74+
.type test_multiple_sethalt,@function
75+
test_multiple_sethalt:
76+
s_sethalt 0
77+
v_add_f32_e32 v0, v1, v2
78+
s_sethalt 0x42
79+
s_endpgm
80+
.size test_multiple_sethalt, .-test_multiple_sethalt
81+
82+
// -- Test 4: kernel without s_sethalt is not modified -----------------------
83+
//
84+
// Kernels that don't contain s_sethalt must round-trip unchanged.
85+
// In particular, no s_nop should be inserted where there wasn't an
86+
// s_sethalt to replace.
87+
//
88+
// CHECK-LABEL: <test_no_sethalt>:
89+
// CHECK-NOT: s_nop
90+
// CHECK: v_add_f32
91+
// CHECK: s_endpgm
92+
.globl test_no_sethalt
93+
.p2align 8
94+
.type test_no_sethalt,@function
95+
test_no_sethalt:
96+
v_add_f32_e32 v0, v1, v2
97+
s_endpgm
98+
.size test_no_sethalt, .-test_no_sethalt

0 commit comments

Comments
 (0)