Skip to content

[Triton/Gluon] [Perf] unified_attention: split KV by actual sequence lengths under CUDA graphs (gfx950, head 64, bf16 Q / fp8 KV) - #7088

Open
yuychang wants to merge 30 commits into
ROCm:mainfrom
yuychang:k3/unified-attn-d64-bf16-fp8-kvsplit
Open

yuychang wants to merge 30 commits into
ROCm:mainfrom
yuychang:k3/unified-attn-d64-bf16-fp8-kvsplit

Conversation

@yuychang

@yuychang yuychang commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Under CUDA graphs, split each request's KV range by its actual length (seqused_k, read on the GPU) instead of the captured max_seqlen_k bound.
  • Scope: gfx950 Triton 3D kernel, bf16 Q, fp8 KV, head dim 64 (Q/K and V), no sliding window. Opt-in through the gfx950 JSON config only.

Motivation

  • SGLang captures decode/verify graphs with max_seqlen_k = 1M, so the uniform split gives every request the same segment count.
  • In a skewed batch (one 120K request among short ones) the long request is the bottleneck: 280–774 µs on main.
  • The launcher cannot read the real lengths on the host without a sync, which a graph forbids.

Changes

  • The kernels compute each sequence's segment count on the GPU from seqused_k (int32 or int64) and run a flat grid of (sequence, segment) work items per KV head.
  • The grid holds split_budget + num_seqs - 1 work items, the most the per-sequence shares can sum to. ITEM_RUN shrinks for small batches.
  • The reduce merges exactly the segments each sequence used.
  • compute_segment_params applies main's SPLIT_FIT_BUDGET / SPLIT_SEQS_PER_TILE in this PR's helper.

New gfx950 config keys (read only on a length-aware launch):

Section / entry Key Value Effect
kv_split D_EQ_64.DT_bf16_fp8 SPLIT_PROGRAMS_PER_CU 4 Opt-in; programs per CU the batch shares
kv_split D_EQ_64.DT_bf16_fp8 SPLIT_SEGMENTS_PER_CU 32 Cap on one sequence's segment count
kv_split D_EQ_64.DT_bf16_fp8 SPLIT_SEQ_BLOCK_MIN / MAX 64 / 256 KV lengths read per step
attn_3d (any, D_LEQ_128.Q_GEQ_4) SPLIT_RUNS_PER_XCD 16 Work-item run length per XCD
attn_3d D_LEQ_128.Q_GEQ_4 SPLIT_BLOCK_M 16 Block sized to one sequence's query rows
reduce SPLIT_SEGMENT_BLOCK_MIN 16 Segments merged per step

The uniform split stays for:

Condition Why
Other dtypes or head dims, asymmetric heads (32,64) / (64,32) Not tuned
skip_reduce Callers read the uniform partial layout
Sliding window The window carve is not tuned for length-aware counts
More than one query block per sequence One q block per sequence is assumed
num_seqs >= 2**15, split_budget + num_seqs >= 2**16, int32 overflow bounds Packed counters and int32 math
Gluon 3D kernel; Gluon reduce The Gluon kernels read the uniform token-major partials

Merged main (#6889 window-aware split, #6983 Q-axis configs): #6983's attn_3d D_LEQ_128.Q_GEQ_4 (BLOCK_M 32) gets SPLIT_RUNS_PER_XCD (without it, q ≥ 4 length-aware calls hit the launcher assert) and SPLIT_BLOCK_M: 16 (BLOCK_M 32 padded q4 blocks to twice their rows; the uniform split keeps BLOCK_M 32).

Performance (if applicable)

Author run: MI355X; bf16 Q / fp8 KV, head 64, page 128; default 8 Q / 2 KV heads. One CUDA graph per workload at max_seqlen_k = 1M; 1 GiB write before each of 40 replays (evicts L2 and MALL); median per round, best of 3 alternating rounds on one GPU. Before = main e4973c4bed, after = 12f033ff57 (later commits change only tests). Kernel latency only.

Hardware Workload/configuration Before (µs) After (µs) Change
MI355X 1×8K, q1, 64 Q / 64 KV heads 40.00 34.82 1.15× faster
MI355X Skewed 32 requests, q4 280.02 51.28 5.46× faster
MI355X Skewed 64 requests, q1 323.96 47.72 6.79× faster
MI355X Skewed 300 requests, q1 774.20 125.14 6.19× faster
MI355X Uniform 64×8K, q4 79.26 69.12 1.15× faster
MI355X Uniform 128×4K, q4 79.22 68.30 1.16× faster
MI355X Uniform 64×64K, q1 236.22 236.26 1.00×
MI355X Uniform 64×64K, q1, 64 Q / 8 KV heads 1031.49 977.33 1.06× faster
MI355X Uniform 8×2K, q1 16.92 18.96 0.89× (+2.04 µs)
MI355X Uniform 256×2K, q1 44.32 46.88 0.95× (+2.56 µs)
  • Skewed batches: one 120K request plus seeded 200–3000-token requests.
  • Trade-off: short-context decode is 5–12% slower, because each workgroup first scans the batch lengths.

SPLIT_BLOCK_M (same method, length-aware launch at BLOCK_M 32 vs 16):

Hardware Workload/configuration BLOCK_M 32 (µs) BLOCK_M 16 (µs) Change
MI355X Skewed 32 requests, q4 55.76 51.14 1.09× faster
MI355X Uniform 64×8K, q4 70.38 69.72 1.01× faster
MI355X Uniform 128×4K, q4 69.24 68.18 1.02× faster
MI355X 56×120K, q4, non-causal (#7244 draft verify) 503.24 422.78 1.19× faster

Review re-run at acd39a137: same script and method, one MI355X, 3 alternating rounds, best of 3. Before = previous head b6997bdce, after = acd39a137 (review fixes). Outputs are bit-identical (same checksum on every workload).

Hardware Workload/configuration Before (µs) After (µs) Change
MI355X 1×8K, q1, 64 Q / 64 KV heads 35.04 34.72 1.01×
MI355X Skewed 32 requests, q4 52.12 52.30 1.00×
MI355X Skewed 64 requests, q1 48.26 48.00 1.01×
MI355X Skewed 300 requests, q1 125.46 126.08 1.00×
MI355X Uniform 64×8K, q4 69.32 69.54 1.00×
MI355X Uniform 128×4K, q4 69.94 69.48 1.01×
MI355X Uniform 64×64K, q1 237.34 237.60 1.00×
MI355X Uniform 64×64K, q1, 64 Q / 8 KV heads 973.03 969.15 1.00×
MI355X Uniform 8×2K, q1 18.84 18.82 1.00×
MI355X Uniform 256×2K, q1 46.90 46.86 1.00×
Benchmark scripts

Run: ./run_bench.sh <gpu> <main-tree> <head-tree> 3

bench_ua_split.py:

"""Graph-replay latency of aiter unified_attention (gfx950, bf16 Q / fp8 KV, head 64, page 128).

Captures one CUDA graph per workload with max_seqlen_k = 1M (the bound sglang
captures), evicts L2 before each replay, and prints the median replay time.
Run it once per tree, e.g. with PYTHONPATH pointing at the tree under test:

    PYTHONPATH=/path/to/aiter python bench_ua_split.py --tag head --out head.json
"""

import argparse
import json
import random
import statistics

import torch

from aiter.ops.triton.attention.unified_attention import unified_attention


def fp8_dtype():
    from aiter.ops.triton.utils.types import e4m3_dtype

    return e4m3_dtype


def skewed(n, q, seed=7):
    rng = random.Random(seed)
    return [(q, 120_000)] + [(q, rng.randint(200, 3000)) for _ in range(n - 1)]


WORKLOADS = {
    "1x8K q1 64/64": ([(1, 8192)], (64, 64)),
    "skewed32 q4": (skewed(32, 4), (8, 2)),
    "skewed64 q1": (skewed(64, 1), (8, 2)),
    "skewed300 q1": (skewed(300, 1), (8, 2)),
    "64x8K q4": ([(4, 8192)] * 64, (8, 2)),
    "128x4K q4": ([(4, 4096)] * 128, (8, 2)),
    "64x64K q1": ([(1, 65536)] * 64, (8, 2)),
    "64x64K q1 64/8": ([(1, 65536)] * 64, (64, 8)),
    "8x2K q1": ([(1, 2048)] * 8, (8, 2)),
    "256x2K q1": ([(1, 2048)] * 256, (8, 2)),
}

DRAFT = {  # Kimi-K3 DSpark draft verify at TP8 (#7244)
    f"{n}x{k // 1000}K q4": ([(4, k)] * n, (8, 2))
    for n, k in [
        (1, 350_000),
        (4, 350_000),
        (16, 120_000),
        (32, 120_000),
        (56, 120_000),
        (56, 30_000),
    ]
}


def build(seq_lens, heads, block_size=128, head=64, causal=True):
    torch.manual_seed(0)
    hq, hk = heads
    qlens = [q for q, _ in seq_lens]
    kvlens = [k for _, k in seq_lens]
    blocks = [(k + block_size - 1) // block_size for k in kvlens]
    nblocks = sum(blocks)
    maxb = max(blocks)
    dt = fp8_dtype()
    q = torch.randn(sum(qlens), hq, head, device="cuda", dtype=torch.bfloat16)
    k = torch.randn(nblocks, block_size, hk, head, device="cuda").to(dt)
    v = torch.randn(nblocks, block_size, hk, head, device="cuda").to(dt)
    perm = torch.randperm(nblocks, device="cuda", dtype=torch.int32)
    bt = torch.zeros(len(seq_lens), maxb, dtype=torch.int32, device="cuda")
    off = 0
    for i, b in enumerate(blocks):
        bt[i, :b] = perm[off : off + b]
        off += b
    cu = torch.tensor([0] + qlens, device="cuda").cumsum(0, dtype=torch.int32)
    seqused = torch.tensor(kvlens, device="cuda", dtype=torch.int32)
    out = torch.empty(sum(qlens), hq, head, device="cuda", dtype=torch.bfloat16)
    one = torch.ones(1, device="cuda", dtype=torch.float32)
    args = dict(
        q=q,
        k=k,
        v=v,
        out=out,
        cu_seqlens_q=cu,
        seqused_k=seqused,
        max_seqlen_q=max(qlens),
        max_seqlen_k=1 << 20,
        softmax_scale=head**-0.5,
        causal=causal,
        window_size=(-1, -1),
        block_table=bt,
        softcap=0,
        q_descale=None,
        k_descale=one,
        v_descale=one,
    )
    return args


def bench(args, replays, flush):
    for _ in range(3):
        unified_attention(**args)
    torch.cuda.synchronize()
    g = torch.cuda.CUDAGraph()
    with torch.cuda.graph(g):
        unified_attention(**args)
    times = []
    for _ in range(replays):
        flush.zero_()
        s, e = torch.cuda.Event(enable_timing=True), torch.cuda.Event(
            enable_timing=True
        )
        s.record()
        g.replay()
        e.record()
        e.synchronize()
        times.append(s.elapsed_time(e) * 1000)
    out = args["out"].float()
    return statistics.median(times), float(out.abs().sum())


def main():
    ap = argparse.ArgumentParser()
    ap.add_argument("--tag", required=True)
    ap.add_argument("--out", required=True)
    ap.add_argument("--replays", type=int, default=40)
    ap.add_argument("--only", nargs="*")
    ap.add_argument("--set", choices=["split", "draft"], default="split")
    ap.add_argument("--non-causal", action="store_true")
    a = ap.parse_args()
    flush = torch.empty(1 << 30, dtype=torch.uint8, device="cuda")  # 1 GiB > L2+MALL
    res = {}
    table = WORKLOADS if a.set == "split" else DRAFT
    for name, (seq_lens, heads) in table.items():
        if a.only and name not in a.only:
            continue
        args = build(seq_lens, heads, causal=not a.non_causal)
        us, checksum = bench(args, a.replays, flush)
        res[name] = {"us": us, "checksum": checksum}
        print(f"{a.tag:6s} {name:18s} {us:9.2f} us", flush=True)
        del args
        torch.cuda.empty_cache()
    json.dump(res, open(a.out, "w"), indent=1)


if __name__ == "__main__":
    main()

run_bench.sh:

#!/usr/bin/env bash
# Alternating main-vs-head rounds on one GPU. usage: run_bench.sh <gpu> <main-tree> <head-tree> [rounds]
G=$1; MAIN=$2; HEAD=$3; R=${4:-3}
D=$(cd "$(dirname "$0")" && pwd)
PY=/dockerx/sglang-agentx-opts/sglang-gpu-perf/kimi_k3/.venv/sglang/bin/python
for r in $(seq 1 $R); do
  for side in main head; do
    T=$MAIN; [[ $side == head ]] && T=$HEAD
    (cd /tmp && HIP_VISIBLE_DEVICES=$G PYTHONPATH=$T AITER_JIT_DIR=/tmp/aiter-jit-bench-$side \
      $PY -I -c "import sys; sys.path.insert(0, '$T'); sys.argv=['b','--tag','$side','--out','$D/bench_${side}_r$r.json']; exec(open('$D/bench_ua_split.py').read())")
  done
done
$PY - "$D" "$R" <<'PY'
import json, sys
d, R = sys.argv[1], int(sys.argv[2])
best = {s: {} for s in ("main", "head")}
for s in best:
    for r in range(1, R + 1):
        for k, v in json.load(open(f"{d}/bench_{s}_r{r}.json")).items():
            best[s][k] = min(best[s].get(k, 1e18), v["us"])
print("| Workload | Main (µs) | Head (µs) | Speedup |\n|---|---:|---:|---:|")
for k in best["main"]:
    m, h = best["main"][k], best["head"][k]
    print(f"| {k} | {m:.2f} | {h:.2f} | {m / h:.2f}× |")
PY

Testing

  • Unit tests added or updated
  • Performance benchmarks run
  • Tested on MI250X
  • Tested on MI300X

MI355X (gfx950), op_tests/triton_tests/attention/test_unified_attention.py:

Revision Selection Passed Skipped Failed
main e4973c4bed whole file¹ 2636 2208 0
this PR f732d9af02 whole file¹ 2649 2208 0
this PR f732d9af02 -k "long_kv or length_aware or graph_replay" 13 0 0
#7244 on top e7fcdde3ee whole file¹ 2997 2208 0
this PR acd39a137 (review run) whole file¹ 2649 2208 0
this PR acd39a137 (review run) -k "long_kv or length_aware or graph_replay or split_scope" 13 0 0

¹ Deselects test_triton_unified_attn_qq_bias_padded_query_rows (5 cases, from #6983); it aborts the process on unmodified main in this environment too.

  • Black and Ruff pass.
  • MI250X / MI300X: not tested; the opt-in entry exists only in the gfx950 config.

Documentation

  • Docstrings updated
  • User guide updated
  • Performance guide updated
  • No documentation change is needed, with the reason stated below

Dependencies

  • No new third-party dependencies added
  • New dependencies are listed and justified below

Breaking Changes

None. Calls outside the scope above keep the uniform split and its outputs.

PR Self-Review

Reviewed revision

d8b4be493098a541154e949f05aa763a94a01c45

d8b4be493 and ede8629bb only shorten comments/docstrings (Copilot review); no code change since acd39a137.

Behavior and intent

  • In scope (gfx950 Triton 3D, bf16 Q, fp8 KV, head 64, no window, one query block per sequence), the KV split follows each request's actual seqused_k instead of the captured max_seqlen_k.
  • Before, a graph captured at 1M gave every request the same segment count, so one long request bounded the whole batch.

Review findings (AITER review-pr + code-review, at b6997bdce)

Finding Result
find_work_item counted empty lanes past num_seqs as sequences before a padding item; near 2**15 sequences the packed count overflows Fixed in acd39a137 (before & valid). Latent today: the 3D path runs only when num_seqs × KV heads ≤ 4 × CUs
The Gluon reduce could be picked for the Triton kernel's slot-major length-aware partials Fixed: use_gluon_reduce requires split_budget is None (latent on gfx950, which has no Gluon reduce)
num_seqs * max_seqlen_k could wrap for a numpy max_seqlen_k Fixed: int() cast
SEQ_BLOCK bounds (64/256) were tuning values in Python (Triton README rule 11) Fixed: SPLIT_SEQ_BLOCK_MIN/MAX in the gfx950 JSON
Config docs said any kv_split entry can opt in Fixed: docs state bf16 Q, head 64 only
Clamp a sequence's segments to its KV tile count Measured, not adopted: within ±1% on 9 of 10 workloads (8×2K q1 18.84 → 18.46 µs)
Replace the ITEM_RUN mapping with the shared remap_xcd helper Measured, not adopted: 1.3–1.8× slower on 7 of 10 workloads (64×64K q1 235.7 → 394.3 µs)
Packed int64 counters vs three plain reductions; one prologue kernel instead of per-program scans Kept: a deliberate per-chunk single reduction; a prologue adds a launch on the short-context path that already pays the scan
A stale max_seqlen_q smaller than a real query length drops rows Not a defect: max_seqlen_q is the documented upper bound; the uniform path also relies on it (2D/3D choice, ALL_DECODE)

Validation and unverified scope

  • Commands: pytest op_tests/triton_tests/attention/test_unified_attention.py -k "not qq_bias_padded_query_rows" and -k "long_kv or length_aware or graph_replay or split_scope" on one MI355X; results in Testing.

  • Benchmarks: run_bench.sh (attached) on one MI355X; results in Performance.

  • End-to-end accuracy through SGLang (Kimi-K3, DSpark draft attention on this path), sgl-eval 0.1.3 run gsm8k --load-preset-from-model-id moonshotai/Kimi-K3 (full 1319, temperature 1.0, top_p 0.95, thinking on, reasoning_effort max):

    Length-aware split GSM8K Truncated Errors
    on (SPLIT_PROGRAMS_PER_CU in the gfx950 config) 97.19%, 97.50% 0 0
    off (key removed for the run) 97.27% 0 0

    Within sampling noise (single-run stderr ~0.45 pp). Setup: 8× MI355X, Kimi-K3-Quark-MXFP4-AttnFP8, AgentX c16 recipe (TP8/DCP8, DSpark block 4, decode graphs bs 1-32), simulated acceptance off. Build: SGLang k3-agentx-pr-stack @ 39d944c57d, aiter stack @ f9b5308b2 (this PR at e2355e700; the later commits are the review fixes and comment edits listed above).

  • Unverified: other architectures.

PR-specific question

  • Consumer: only gfx950 Triton kv_split D_EQ_64.DT_bf16_fp8 sets SPLIT_PROGRAMS_PER_CU; its uniform values equal D_LEQ_64.
  • The attn_3d keys (SPLIT_RUNS_PER_XCD, SPLIT_BLOCK_M) and reduce key (SPLIT_SEGMENT_BLOCK_MIN) are read only on a length-aware launch.
  • Other shapes: guards (dtype, head dims, window, skip_reduce, query block, integer bounds) return the uniform split. test_length_aware_split_scope checks this and that attn_3d/reduce entries for q = 1, 4, 8 carry their keys.

Changed op tests

op_tests/triton_tests/attention/test_unified_attention.py:

  • test_triton_unified_attn_3d_long_kv: actual vs captured KV bound.
  • test_triton_unified_attn_3d_length_aware_split: skewed, uniform, 300-request, int64 seqused_k and q=8 batches against ref_paged_attn, plus per-sequence segment counts.
  • test_triton_unified_attn_3d_length_aware_split_graph_replay: changes seqused_k between graph replays.
  • test_length_aware_split_scope: excluded calls, asymmetric heads, sliding window, integer bounds, the query-block guard on verify params (q = 4 and 8 split, q = 9 falls back), config keys, and the SEQ_BLOCK the entry gives.
  • Not redundant: no existing 3D test covers captured bounds or per-sequence segment allocation.

@yuychang
yuychang requested review from a team and a balanced review from Copilot October 28, 2026 09:27
@github-actions github-actions Bot changed the title [Triton] unified_attention: gfx950 kv_split for head-dim-64 bf16-Q / fp8-KV [Triton/Gluon] [Config] unified_attention: gfx950 kv_split for head-dim-64 bf16-Q / fp8-KV Oct 28, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🏷️ CI Guide

Runs automatically on every PR:

  • ✅ Pre-checks (submodule verification, code formatting)
  • ✅ Aiter op tests (gfx942 + gfx950)
  • ✅ Triton tests on MI35X (only when aiter/ops/triton/** or related paths are changed)

Extended tests (opt-in via labels):

Label Tests
ci:gfx1250-ffm-triton Run the five-shard gfx1250 FFM Triton test suite
ci:triton-300x Run an additional Triton test job on MI300X in PRs (added automatically when gfx942 configs change); main branch always runs both MI35X and MI300X
ci:triton-355 Run the full Triton test suite on MI35X, not only the tests the change affects
ci:windows Windows JIT op smoke test on the gfx1201 ROCm runner
ci:multi-gpu Aiter multi-GPU tests on the 8-GPU runner
ci:sglang SGLang integration tests: DeepSeek-R1-MXFP4 accuracy, Qwen 3.5 accuracy
ci:atom ATOM benchmark: DeepSeek-R1-0528, GPT-OSS-120B
ci:atom_full ATOM accuracy suite for PR and main models from ATOM models_accuracy.json
ci:vllm vLLM benchmark: DeepSeek-V4-Pro, MiniMax-M3-MXFP4
ci:all All standard extended tests (excludes ci:atom_full)

Only add ci:atom_full for FlyDSL or Triton upgrades.
Add labels via the sidebar or gh pr edit 7088 --add-label <label>

One backend per PR:
A PR changes one kernel backend: [Triton/Gluon] (Triton and Gluon count as one), [HIP], [ASM], [CK], [OPUS] or [FlyDSL]. If the title ends up with two backend tags, split the PR -- as stacked pull requests when one part cannot merge without the other.

PR title tags & labels:
Component tags ([Triton/Gluon], [HIP], [CK], [ASM], ...) are added to the PR title and as PR labels automatically from the changed files and re-synced on every push — change-type tags like [fix]/[Perf], op tags like [MLA], and human labels (ci:*) are left untouched. Add the no-auto-title label to stop the title rewrites; labels stay in sync either way.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The gfx950 split-count retune needs exact-head CUDA-graph correctness and latency validation because the supplied measurements predate this PR and GPU execution was unavailable during review.

No open issues 🎉

What changed in this PR

This config-only PR tunes unified attention on gfx950 for head dimensions up to 64 with bf16 queries and fp8 KV, targeting long-context decode under CUDA graphs.

Changes:

  • Adds a dtype-specific entry with 128 KV segments and fixed segment and tile sizes of 64.
  • Leaves kernel code unchanged. Reported measurements show faster long-context decode, with a 2.6 µs slowdown for the short-sequence case.
File Description
aiter/​ops/​triton/​configs/​gfx950/​triton/​attention/​unified_attention/​DEFAULT.json Adds the D_LEQ_64.DT_bf16_fp8 KV-split configuration.

Review effort: Balanced

@github-actions github-actions Bot changed the title [Triton/Gluon] [Config] unified_attention: gfx950 kv_split for head-dim-64 bf16-Q / fp8-KV [Triton/Gluon] unified_attention: gfx950 kv_split for head-dim-64 bf16-Q / fp8-KV Oct 28, 2026
@github-actions github-actions Bot removed the Config label Oct 28, 2026
Copilot AI balanced review requested due to automatic review settings October 28, 2026 14:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The long-KV test tolerances can accept zero for small reference values, potentially masking lost split contributions.

1 open issue

Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 28, 2026 15:05

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Human assessment is needed because a finding that passed the severity filter was omitted from the published review.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

@yuychang yuychang changed the title [Triton/Gluon] unified_attention: gfx950 kv_split for head-dim-64 bf16-Q / fp8-KV [Triton/Gluon] [Perf] unified_attention: gfx950 kv_split for head-dim-64 bf16-Q / fp8-KV Oct 28, 2026
@yuychang
yuychang requested a balanced review from Copilot October 28, 2026 16:41

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The test docstring redundantly repeats setup values and exceeds the documented short-comment limit.

1 open issue

Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 28, 2026 16:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The gfx950 default’s reported mixed-length gains come with uniform-batch slowdowns, requiring maintainer judgment supported by target-GPU correctness and latency measurements unavailable in this review.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 28, 2026 17:12

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The gfx950 tuning’s reported long-context gains come with uniform-batch latency regressions, requiring maintainer judgment on the workload trade-off because GPU correctness and performance results could not be independently reproduced.

No open issues 🎉

Review effort: Balanced

@zufayu
zufayu requested review from a team and Dewei-Wang-sh October 29, 2026 01:03
@yuychang
yuychang force-pushed the k3/unified-attn-d64-bf16-fp8-kvsplit branch from 4948cd3 to be2a245 Compare October 29, 2026 02:06
@yuychang
yuychang force-pushed the k3/unified-attn-d64-bf16-fp8-kvsplit branch from f732d9a to b6997bd Compare November 4, 2026 04:33
- find_work_item: count only valid lanes as sequences before an item, so a padding item cannot overflow the packed count
- Keep the Gluon reduce off when the Triton 3D kernel wrote slot-major length-aware partials
- Cast max_seqlen_k to int in the int32 bound check, so a numpy scalar cannot wrap the product
- Move the SEQ_BLOCK bounds from Python into the gfx950 kv_split entry (SPLIT_SEQ_BLOCK_MIN/MAX)
- State in the config docs that the length-aware split covers bf16 Q with head size 64 only
@azaidy
azaidy requested a review from cagrikymk November 4, 2026 08:41
Copilot AI balanced review requested due to automatic review settings November 4, 2026 08:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Four comments and docstrings exceed the project's two-sentence limit and need to be shortened.

4 open issues

Review effort: Balanced

yuychang added a commit to yuychang/aiter that referenced this pull request Nov 4, 2026
…n-noncausal

- Pick up the length-aware split fixes from ROCm#7088 (item lookup, Gluon reduce gate, int bound, SEQ_BLOCK config)
- Condense the kernel block comment, _length_aware_split docstring, utils paragraph and test helper docstring.
- Follows the one-or-two-sentence comment rule in .github/instructions/aiter-ops-triton.instructions.md.
Copilot AI balanced review requested due to automatic review settings November 4, 2026 08:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The gfx950 indexing and partial-buffer changes require independent GPU correctness, CUDA-graph replay, and base/head latency validation that was unavailable in this source-only review.

No open issues 🎉

4 resolved since last review

Review effort: Balanced

- Keep the work-item bound and why ITEM_RUN groups heads by XCD (Copilot review on ROCm#7244).
Copilot AI balanced review requested due to automatic review settings November 4, 2026 09:02

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Human review is needed to assess the impact of distinct split-block schedules sharing a trace name despite documented schedule-visibility requirements.

No open issues 🎉

Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The packed work-item mapping and slot-major reduction require exact-head gfx950 correctness and performance validation under CUDA graph replay, which this source-only review cannot perform.

No open issues 🎉

Review effort: Balanced

… reduce

- reduce_segments: keep tiles_per_segment >= 1, so a zero-length sequence does not compute cdiv(0, 0)
- Length-aware reduce: cap the merged slots at NUM_SEGMENTS_PER_SEQ, the most slots the 3D kernel writes
- Length-aware reduce: get num_tokens from the launcher for the slot stride, as the 3D kernel does
- Tests: add skewed batches with zero-length (padded) requests, and a padded replay to the graph test
- Tests: fill the output with NaN before the call, and require zero rows for a zero-length request
- _length_aware_split needs all kv_split, attn_3d and reduce keys the split reads, else it returns None
- Remove the NUM_SEGMENTS fallback for SPLIT_NUM_SEGMENTS and the tuning values in the SEQ_BLOCK defaults
- Return a _LengthAwareSplit with the attn_3d and reduce entries, so the launchers do not read them again
- Share the BLOCK_M rule of the 3D launch and the query-block guard in _attn_3d_block_m
- Eager host time per in-scope call on MI355X: 8x2K q1 102.3 -> 103.4 us, skewed32 q4 108.0 -> 104.3 us
- Tests: check the fallback for each missing key, and pin the attn_3d and reduce lookups to gfx950
- Replace use_sinks with an option parameter: sinks, shuffled_kv_cache, softcap or qq_bias
- Add skewed q4 cases for shuffled_kv_cache, softcap and qq_bias, which the length-aware guard does not exclude
- Use softcap 0.1: the scores here are ~0.1, so a dropped softcap moves each request ~5%, past the 2e-2 bound
- Size the cache as the sum of per-request pages plus 64 spare pages instead of requests x the longest request
- skewed_300 drops from 60.2 GiB to 1.3 GiB peak GPU memory (it allocated ~37 GB of fp32 K/V for ~5K used pages)
- Each request still gets distinct random pages; the graph replay test keeps each request's pages across replays

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The GPU allocation and slot-major reduction changes require exact-head gfx950 CUDA-graph correctness and base/head latency validation that this source-only review cannot perform.

No open issues 🎉

Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The docstring in op_tests/triton_tests/attention/test_unified_attention.py:675 exceeds the project's two-sentence limit and needs shortening.

1 open issue

Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The new GPU work-item mapping and reduction layout require exact-head gfx950 correctness and CUDA-graph replay validation, which this source-only review cannot provide and no supplied report establishes.

1 open issue

Review effort: Balanced

- The Triton review rules allow one or two sentences per docstring

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The new GPU work-item mapping and slot-major reduction require independent exact-head gfx950 correctness, latency, and changing-length graph-replay validation that this source-only review could not perform.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

…on gfx942

- The length-aware split tests pin the gfx950 kv_split tables on gfx942, like test_length_aware_split_scope does
- Its kernels are plain Triton, so gfx942 CI now checks the split math the gfx950 entry relies on
- test_triton_unified_attn_3d_long_kv accepts gfx942; all 19 cases pass on MI300X
@yuychang

yuychang commented Nov 6, 2026

Copy link
Copy Markdown
Contributor Author

gfx942 (MI300X) validation: validate-kernel-pr PASS.

1149 passed / 0 failed; independent grid 317 pass. The length-aware split is gated off on gfx942 by its config and runs correctly there when the gfx950 tables are pinned; the split and long-KV tests now run on gfx942. Bench latency within ±0.8% of base.

New commits for gfx942: 4efe461.

Reports were generated with the repo's validate-kernel-pr skill on a local head, so repo.head is null and they are not attached as PR evidence.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The packed work-item mapping and slot-major reduction require head-matched gfx950 correctness, graph-replay, and latency validation that the source-only reviews could not perform.

No open issues 🎉

Review effort: Balanced

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants