Compile MLIR prefills with tuning candidates - #5268
umangyadav wants to merge 4 commits into
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
🟡 Changes recommended
Add enabled regression coverage for the new prefill integration path.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR moves MLIR prefill compilation into parallel tuning-candidate compilation and removes serial benchmark-time compilation.
Changes:
- Carries compiled prefill code into replacement callbacks.
- Handles flattened tuple-output allocation shapes.
- Inserts fills before associated MLIR kernels.
File summaries
| File | Description |
|---|---|
src/targets/gpu/jit/mlir.cpp |
Compiles and inserts MLIR prefill operations. |
src/targets/gpu/compile_ops.cpp |
Removes serial device-op compilation during benchmarking. |
The new path lacks an enabled GPU MLIR regression covering prefilled candidates and tuple-output allocation.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Tuple-output prefills can be eliminated before benchmarking, and the integration lacks enabled regression coverage.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
src/targets/gpu/jit/mlir.cpp:301
- The new compiled-prefill integration has no enabled regression test. The only MLIR prefill tests in
test/gpu/mlir.cppare skipped, while the existinghip::filltests exercise the compiler only in isolation; therefore incorrect prefill-index mapping, tuple-output flattening, or code-object offsets can pass CI. Add an enabled GPU MLIR test that compiles and executes a prefilled candidate with a tuple output allocation.
return migraphx::gpu::compile_op(
fill.name(), ctx, {inputs.at(mco.prefill_indices.at(i))}, fill.to_value());
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The modified split-candidate prefill path lacks deterministic regression coverage.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/targets/gpu/jit/mlir.cpp:393
- The split fallback's new prefill compilation is not covered by the added regression.
prefill_tuple_outputrequires exactly one MLIR code object plus two fills (code_objects.size() == 3), while this branch starts with two MLIR code objects, so it cannot validate this path or themcos.size() + ireplacement offset. Add a case that deterministically forcesis_module_fusibleto return false and verifies the split candidate's fill is compiled, retained, and executed before the first MLIR kernel.
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Balanced
| result.reserve(indices.size()); | ||
| std::transform( | ||
| indices.begin(), indices.end(), std::back_inserter(result), [&](const auto i) { | ||
| auto fill = migraphx::make_op("hip::fill", {{"value", mco.prefill_values.at(i)}}); |
There was a problem hiding this comment.
is this cached? I guess fill kernels (with the same shapes) will be repeated over and over. I'd be nice to have a dynamic shape fill kernel if the dynamic functionality is ready, so it gets compiled only once.
There was a problem hiding this comment.
It is not cached. I think #5270 addresses it right ?
There was a problem hiding this comment.
yes, I got the idea while reviewing the code here. The results on that PR are on top of this PR.
|
Closing this one. #5275 |
Problem
The MLIR tuning path compiled inserted
hip::filloperations fromtime_solution(). Since candidate benchmarking is serial, this moved fill compilation out of the parallel candidate-compilation phase.Follow-up to #5252 and the maintainer feedback in #5252 (comment).
Changes
Validation
python3 tools/format.py origin/develop --clang-format-path /opt/rocm/llvm/bin/clang-formatcmake --build build --target analyze --parallel 32git diff --check origin/develop...HEADCppcheck was not available in the local build environment.
Made with Cursor