[2/5] autotune: two-track config + first real adopter (rmsnorm) (#770)#785
[2/5] autotune: two-track config + first real adopter (rmsnorm) (#770)#785jhinpan wants to merge 16 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends FlyDSL’s autotuning system to support a two-track “default heuristic vs forced search” flow and introduces a first real adopter (RMSNorm) that exercises builder-mode autotuning (rebuilding modules per structural config).
Changes:
- Add builder-mode support to
Autotuner(build_fn-based rebuild + in-process build cache) and introduce a heuristicdefault=path that skips search unlessFLYDSL_AUTOTUNE=1. - Extend autotune machinery with stride normalization + extra cache-key axes, and add unit tests covering builder/default behavior.
- Add RMSNorm autotune integration (
rmsnorm_config.py+rmsnorm_autotune.py) and a GPU integration test.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
python/flydsl/autotune.py |
Adds builder-mode execution, two-track default/forced-search behavior, and additional cache key axes; updates benchmark/run path accordingly. |
kernels/rmsnorm_kernel.py |
Adds a build-time BLOCK_THREADS knob and applies known_block_size when needed. |
kernels/rmsnorm_config.py |
Introduces heuristic default + exhaustive config generation for RMSNorm tuning. |
kernels/rmsnorm_autotune.py |
Provides the tuned RMSNorm front-end using builder-mode autotuning. |
tests/unit/test_autotune.py |
Adds GPU-free unit tests for builder mode, default skip/force behavior, kwarg filtering, and env gating. |
tests/kernels/test_rmsnorm_autotune.py |
Adds an end-to-end GPU test validating default correctness, forced search, persistence, and cache reuse. |
Comments suppressed due to low confidence (1)
python/flydsl/autotune.py:250
- In builder mode (fn=None), the disk-cache filename falls back to "unknown.json", so different tuners in the same cache dir can clobber each other’s tuned results. Consider deriving the cache filename from
fnwhen present, otherwise frombuild_fn(and include the module name) so each tuner has an isolated cache file.
# Disk cache
fn_name = getattr(fn, "__name__", None) or getattr(fn, "func", None)
if fn_name is not None and not isinstance(fn_name, str):
fn_name = getattr(fn_name, "__name__", "unknown")
fn_name = fn_name or "unknown"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Dtypes + normalized strides of tensor args for type/layout specialization | ||
| dtype_parts = [] | ||
| stride_parts = [] | ||
| for name, val in sig_args.items(): | ||
| if hasattr(val, "dtype"): | ||
| dtype_parts.append(f"{name}:{val.dtype}") | ||
| if hasattr(val, "shape") and hasattr(val, "stride"): | ||
| stride_parts.append(f"{name}:{_normalize_strides(val)}") | ||
| key_vals.append(tuple(dtype_parts)) | ||
| key_vals.append(tuple(stride_parts)) |
| def kernel_call(): | ||
| if config.pre_hook: | ||
| config.pre_hook(merged_kwargs) | ||
| self._restore_tensors(snapshot) | ||
| self._reset_tensors(args, merged_kwargs) |
| compiler_opts = config.compiler_opts() | ||
| fn = self._resolve_fn(config, key, args, kwargs) | ||
| merged_kwargs = dict(self._filter_call_kwargs(fn, kwargs)) | ||
| # In builder mode the config's structural kwargs (e.g. BLOCK_THREADS) | ||
| # are consumed by build_fn, not passed to the launch call. | ||
| if self.build_fn is None: | ||
| merged_kwargs.update(config.all_kwargs()) | ||
|
|
| """Run a single (already-chosen) config: resolve fn, apply kwargs+hints.""" | ||
| fn = self._resolve_fn(config, key, args, kwargs) | ||
| merged = dict(self._filter_call_kwargs(fn, kwargs)) | ||
| if self.build_fn is None: | ||
| merged.update(config.all_kwargs()) | ||
| return self._run_with_hints(fn, config.compiler_opts(), args, merged) |
FlyDSL's autotuner exists but nothing uses it, and two gaps block real
adoption. This is the first of a series making it a correct, adopted path.
Cache key (_make_key) previously specialized on shape/dtype only. A config
tuned under one compiler build, GPU arch, or memory layout would be silently
reused under another. Fold in the axes Triton/quack rely on:
- normalized stride pattern ({0,1,other}: broadcast vs contiguous vs strided)
- device arch (get_rocm_arch)
- toolchain fingerprint (reuses jit_function._flydsl_key)
- cache-invalidating env vars (reuses _cache_invalidating_env_values)
The dtype/stride axes are sorted by arg name so a call is keyed identically
regardless of kwarg order (no duplicate tuning / cache files).
restore_value (new) is the correctness soul of autotune: benchmarking runs
the same kernel dozens of times, so an in-place / accumulating kernel (e.g.
fused-add rmsnorm) corrupts its own inputs and picks a config on garbage.
Snapshot the named tensors once and restore before every rep.
reset_to_zero is now also re-applied on the real (non-benchmark) call — both
the post-tune run and cache hits — via a shared _run_config, so an
accumulate-into-zero kernel returns the single-clean-run result instead of
carrying benchmark-rep state. (Was applied only inside the bench loop.)
Also defer the CompilationContext import so the autotuner core stays
importable and unit-testable without the compiled flydsl._mlir bindings.
Adds tests/unit/test_autotune.py: 19 GPU-free tests covering Config
serialization, every cache-key axis (incl. env-fingerprint change and
kwarg-order insensitivity), restore_value/reset_to_zero semantics (incl. the
final-run and cache-hit reset), pruning, and disk-cache round-trip.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
eb94a0a to
0aa86e0
Compare
e96dad5 to
db07800
Compare
Comment-only cleanup of the PR1 additions: keep the one key fact per helper, drop the Triton/quack background, redundant restatements, and by-example prose. No logic change; 19 unit tests still pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
db07800 to
95ea147
Compare
Second in the series. Gives the autotuner an "avoid-search" path and puts it to work on a real kernel, so it stops being dead code. Builder mode. Every current FlyDSL kernel bakes its structural knobs at module-build time rather than exposing them as jit Constexpr params. The existing @autotune, which injects config kwargs into one jit call, can't tune those. Add builder mode: build_fn(config, *args) -> launch_callable rebuilds the module per config. autotune_builder(): one-call adoption. Instead of a hand-rolled wrapper per kernel, a kernel opts in with just its kernel-specific pieces: rmsnorm_autotuned = autotune_builder( name="rmsnorm", build=build_rmsnorm_module, specialize=lambda inp, g, out, m, dtype_str="bf16", **kw: { "N": inp.shape[-1], "dtype_str": dtype_str}, configs=get_all_configs, default=get_default, structural=("BLOCK_THREADS",)) The helper owns cache naming, callable config generation, the structural-vs- compiler knob split, build caching, and launch-kwarg filtering. rmsnorm's adopter dropped from ~79 lines of boilerplate to a single declaration. Correctness (surfaced by two independent fresh reviews): - Build cache keys only on the STRUCTURAL knobs + spec, not repr(config), so configs differing only in a compiler hint (waves_per_eu) reuse one built module. Verified on MI350X: a 12-config sweep now builds 4 modules, not 12. - A build-only scalar (dtype_str) passed positionally is rejected with a clear error instead of silently binding to the wrong launch slot (e.g. stream). - autotune_builder requires a non-empty name (empty/None would fall back to unknown.json and defeat the per-kernel cache identity). - Build-only scalars enter the cache key via the specialize() axes. - Compiler hints (waves_per_eu / maxnreg) are folded into the JitFunction's compile_hints (restored after) so each distinct hint compiles a distinct binary instead of reusing a cached one. - FLYDSL_AUTOTUNE=1 bypasses cache + default to force a fresh search. - Disk cache is re-loaded when FLYDSL_AUTOTUNE_CACHE_DIR changes (a module- level tuner isn't pinned to the import-time dir for loads or saves). - Config.pre_hook is documented as non-persistable (not serialized). Two-track config == Triton @Heuristics + @autotune: default= gives zero-search normal runs; FLYDSL_AUTOTUNE=1 forces the sweep. First adopter rmsnorm: build_rmsnorm_module gains a BLOCK_THREADS build knob (+known_block_size when >256; default arg keeps the old signature). config space in rmsnorm_config.py; small-N (imported SMALL_N_THRESHOLD) emits a single config since that kernel ignores BLOCK_THREADS. VEC_WIDTH stays pinned (128b copy). Verified on MI350X (gfx950), M=4096 N=8192 bf16: default and forced-search match the torch reference (max err 1.5e-2); sweep picks BLOCK_THREADS 256-512 and a subsequent normal call reuses the cache (zero benchmark invocations asserted). Tests: GPU-free unit tests for builder mode + autotune_builder (rebuild/cache, build-cache-ignores-hints, default skip/force, scalar-in-key, name required, positional-scalar rejected) = 31; tests/kernels/test_rmsnorm_autotune.py (l2_device) covers default, forced-search, and no-re-tune cache reuse. ruff + black clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
95ea147 to
1bc2ee7
Compare
The autotuner routes Config(waves_per_eu=...) through compile_hints -> --amdgpu-waves-per-eu on gpu-module-to-binary opts=, which the AMDGPU backend silently ignores (a known limitation). So the rmsnorm _WAVES_PER_EU_CHOICES sweep tuned nothing. Lower the waves_per_eu compile-hint onto the kernel's rocdl.waves_per_eu gpu.func attribute in MlirCompiler.compile (before convert-gpu-to-rocdl) -- the same path the MoE kernels use. convert-gpu-to-rocdl turns it into the LLVM amdgpu-waves-per-eu function attribute, which the backend honors. Verified on MI355X (gfx950): the compiled LLVM IR now carries "amdgpu-waves-per-eu"="N" (previously absent) and the backend acts on it (ISA changes; occupancy warning at aggressive values). The JIT compile cache key already includes compile_hints, so each waves_per_eu recompiles a distinct binary. Co-authored-by: Cursor <cursoragent@cursor.com>
1e07aff to
a19f7e6
Compare
Follow-up to the waves_per_eu fix: the same "silently dropped compile hint" bug hit maxnreg, and num_warps had a related builder-mode trap. - maxnreg: Config(maxnreg=...) reached codegen only via --amdgpu-num-vgpr on gpu-module-to-binary opts=, which the AMDGPU backend ignores. Unlike waves_per_eu, amdgpu-num-vgpr has no native ROCDL translation, so lower it onto the gpu.func LLVM `passthrough`; convert-gpu-to-rocdl copies that to the llvm.func where the emitter turns it into the amdgpu-num-vgpr function attribute (same approach as FlyROCDLClusterAttrPass for amdgpu-cluster-dims). Generalize _apply_occupancy_compile_hints to a hint -> func-attr lowering. Verified on MI355X: the IR now carries "amdgpu-num-vgpr"="N" and the backend acts on it (ISA changes when the cap binds, e.g. 40 < the kernel's 60 VGPRs; 64/128 don't bind); outputs stay correct. - num_warps: in builder mode the block size is baked into build_fn, so a jit-kwarg num_warps can't be routed to the launch call and was silently dropped. Reject it with a clear error instead of tuning a no-op. - Tests: assert the func-attr lowering and the builder-mode num_warps rejection. Co-authored-by: Cursor <cursoragent@cursor.com>
…cache, f32) - Thread-safety: _run_with_hints no longer mutates the shared, cached fn.compile_hints (a race across concurrent tuned/served calls); it sets only the thread-local CompilationContext, and JitFunction folds the thread-local hints into its cache key + compile push, so each distinct waves_per_eu/maxnreg still compiles a distinct binary. - Search loop no longer silently swallows failures: unroutable configs (num_warps in builder mode) are rejected up front so they fail loudly even in a forced sweep, and "all configs failed" chains the last underlying error. - Disk cache: atomic write (tmp+rename); a single malformed entry is skipped rather than discarding the whole cache; a stale cached entry degrades to the default instead of crashing a normal call. - Cache key folds a source fingerprint of the adopter build/config functions, so editing the kernel/config invalidates a stale tuned best. - maxnreg lowering (_set_passthrough) replaces an existing amdgpu-num-vgpr entry instead of appending a duplicate. - restore_value/reset_to_zero run as an untimed per-rep setup (do_bench gains a setup param), so the restore copy no longer inflates the measurement. - get_all_configs sweeps f32 too (scalar path is BLOCK_THREADS-strided; it was silently collapsing to the single default). Verified on MI355X: f32 configs build and match reference (max_err 2.9e-6). - Builder mode rejects config kwargs not in structural (would route nowhere). Adds unit tests for each behavior. Co-authored-by: Cursor <cursoragent@cursor.com>
|
/rerun-ci |
|
/rerun-ci |
…obustness) Follow-ups from the second multi-model review (round-1 fixes verified): - Cache-hit fallback catches only config-incompatibility errors (ValueError/ TypeError/KeyError); genuine compile/launch/runtime errors now propagate instead of being masked. Logs and drops the stale entry from disk too. - _call_do_bench passes `setup` only when the benchmarker explicitly declares it -- a **kwargs catch-all no longer gets setup passed-and-silently-dropped (which would skip restore/reset). - _save_disk_cache is best-effort: a write failure (read-only FS, full disk) is logged, never crashes an otherwise-successful tune. - _iter_gpu_kernel_funcs applies occupancy hints only to entry-point kernels (gpu.kernel attr), skipping non-kernel device helpers. - _source_fingerprint hashes the source file (transitive over module-level helpers/constants), not just the function body. - Tests: **kwargs do_bench folds setup; cache-hit degrades to default on a stale config; f32 get_all_configs sweeps BLOCK_THREADS. Co-authored-by: Cursor <cursoragent@cursor.com>
|
/rerun-ci |
|
/rerun-ci |
|
/rerun-ci |
Two ir.Module.parse(...) calls were left wrapped multi-line (non-canonical under black's 120-col config, likely after a merge-conflict resolution), which failed the "Check Python Code Style" job. Collapse them to a single line via black. No logic change; ruff was already clean. Co-authored-by: Cursor <cursoragent@cursor.com>
|
/rerun-ci |
| func_op.attributes["passthrough"] = ir.ArrayAttr.get(kept + [entry]) | ||
|
|
||
|
|
||
| def _apply_occupancy_compile_hints(module: ir.Module) -> None: |
There was a problem hiding this comment.
Seems we have three methods to set occupancy? Passing gpu-module-to-binary, setting attributes ( in moe) ? We only need one finally.
There was a problem hiding this comment.
Good catch — agreed there should be exactly one. Plan: consolidate onto the gpu.func attribute mechanism (the only one the AMDGPU backend actually honors) and drop the rest.
gpu-module-to-binary opts=(--amdgpu-waves-per-eu/--amdgpu-num-vgprinbackends/rocm.py) is the dead path — the backend silently ignores these (as measured earlier in this thread), yet the autotuner was still feeding the same hint into it in parallel with the func-attr lowering. So the hint was actually consumed twice. Removing it so it's consumed in exactly one place. (A unit test intest_external_llvm_codegen.pyeven asserted these land inopts=; flipping it to assert they don't, to lock the no-op out.)- hand-set
value_attrsin MoE/attention and this autotune hint-lowering are really the same mechanism — both end up writingrocdl.waves_per_eu/amdgpu-num-vgpronto the kernelgpu.func. I'll factor the knob→attribute mapping into a single_set_occupancy_attrs()helper so there's one source of truth;_apply_occupancy_compile_hintsroutes through it (hand-authored kernels can converge onto it later too).
Net after the change: occupancy hints are consumed in one place, and a single helper defines how a knob becomes a gpu.func attribute.
|
_apply_occupancy_compile_hints applies the same waves_per_eu / maxnreg hints to every gpu.kernel entry point in the module. That’s fine for single-kernel launchers like rmsnorm, but it’s a limitation worth documenting: if a @jit launcher ever emits multiple entry kernels that need different occupancy settings, autotune can’t tune them independently — one Config(waves_per_eu=...) would blanket all kernels. |
waves_per_eu / maxnreg were consumed twice: once lowered onto the kernel gpu.func as attributes (the mechanism the AMDGPU backend honors) and once as --amdgpu-waves-per-eu / --amdgpu-num-vgpr on gpu-module-to-binary opts=, which the backend silently ignores. The latter is a dead no-op, so the same hint was being consumed on two paths. - rocm backend: stop forwarding occupancy knobs to gpu-module-to-binary opts= so the hint is consumed in exactly one place. - jit_function: extract _set_occupancy_attrs() as the single source of truth for the knob -> gpu.func attribute mapping; _apply_occupancy_ compile_hints() routes through it, and hand-authored value_attrs kernels land on the same attribute. - test: flip test_rocm_external_pipeline_split_matches_full_pipeline to assert occupancy knobs are absent from opts=, locking out the no-op. Addresses review feedback on ROCm#785 (only one occupancy mechanism). Co-authored-by: Cursor <cursoragent@cursor.com>
waves_per_eu / maxnreg were consumed twice: once lowered onto the kernel gpu.func as attributes (the mechanism the AMDGPU backend honors) and once as --amdgpu-waves-per-eu / --amdgpu-num-vgpr on gpu-module-to-binary opts=, which the backend silently ignores. The latter is a dead no-op, so the same hint was being consumed on two paths. - rocm backend: stop forwarding occupancy knobs to gpu-module-to-binary opts= so the hint is consumed in exactly one place. - jit_function: extract _set_occupancy_attrs() as the single source of truth for the knob -> gpu.func attribute mapping; _apply_occupancy_ compile_hints() routes through it, and hand-authored value_attrs kernels land on the same attribute. - test: flip test_rocm_external_pipeline_split_matches_full_pipeline to assert occupancy knobs are absent from opts=, locking out the no-op. Addresses review feedback on ROCm#785 (only one occupancy mechanism).
4b253f8 to
2a1384e
Compare
Second in the #770 series: give the autotuner an avoid-search path and put it to work on a real kernel, so it stops being dead code.
Builder mode
Every current FlyDSL kernel bakes its structural knobs at module-build time — they size shared storage, the vectorized tile stride, and the launch block, and
known_block_sizeis fixed at@kerneldecoration — rather than exposing them as jitConstexprparams. The existing@autotune, which injects config kwargs into a single jit call, can't tune those.So this adds builder mode:
build_fn(config, *args, **kwargs) -> launch_callablerebuilds the module per config; built modules are cached per(key, config). Structural config kwargs route tobuild_fn(not the launch call); build-only kwargs likedtype_strare filtered out of the launch call by signature.Two-track config
== Triton
@heuristics+@autotune, quackget_default+ exhaustive. Newdefault=heuristic on@autotune: normal runs take the analytic default and skip the search entirely (zero-search); setFLYDSL_AUTOTUNE=1to force the full sweep. Autotuners without a default always search.First adopter: rmsnorm
build_rmsnorm_modulegains aBLOCK_THREADSbuild knob (addsknown_block_sizewhen >256); the default arg keeps the old signature working, so the existing rmsnorm test path is unchanged.kernels/rmsnorm_config.py—get_default(analyticBLOCK_THREADS) +get_all_configs(BLOCK_THREADS×waves_per_eu, fast-path only).VEC_WIDTHis intentionally not tuned (pinned to128/elem_bitsby the 128-bit buffer copy).kernels/rmsnorm_autotune.py—rmsnorm_autotuned, the tuned front end.Verification (MI350X / gfx950, M=4096 N=8192 bf16)
Default and forced-search runs both match the torch reference (max err 1.5e-2). The 12-config sweep runs cleanly and picks
BLOCK_THREADS256–512; the result is cached (second call doesn't re-tune):Tests
tests/unit/test_autotune.py.tests/kernels/test_rmsnorm_autotune.py(l2_device): default run, forced-search + tuned-config persisted, cache reuse.ruff + black clean.
Refs #770.
🤖 Generated with Claude Code