Fix seg fault when splitting non-fusible MLIR modules - #5187
Fix seg fault when splitting non-fusible MLIR modules#5187justinrosner wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a GPU compilation crash when rocMLIR reports a fused gpu::mlir_op module as non-fusible and MIGraphX falls back to splitting the fused module. The change hardens split-point selection (find_final_split) to avoid invalid iterator dereferences, and adds a GPU regression test that exercises the previously-crashing split paths.
Changes:
- Add guards in
find_final_split()to handle short output paths and the “no split edge found” case without dereferencingend(). - Add a new GPU test that compiles fused MLIR modules under a non-fusible split-K tuning configuration to ensure no split-time crash.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/targets/gpu/jit/mlir.cpp |
Adds defensive handling in split-point selection to prevent iterator invalidation/dereference crashes during MLIR module splitting. |
test/gpu/mlir_split_fusion.cpp |
Adds GPU regression coverage for split-K non-fusible MLIR module splitting paths that previously could segfault. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Regressions detected 🔴 * No develop baseline was found for this PR's branch point; compared against the latest available develop run instead. |
|
|
I dont know about the unit tests, but can you add the verify tests that you had in the other PR? That got us coverage as well for this. |
I assume you are talking about the tests from #5064? I'm not sure if they would actually give us coverage for the fixes in this PR. Those tests cover picking the right split point when one exists, whereas this PR covers the cases where there isn't one. |
OK but I dont see how this would happen as it should always have a gemm or convolution. And the unit tests show cases that are pointwise-only which would never happen. |
The unit tests all have a GEMM (they all call the |
Motivation
When compiling fused
gpu::mlir_opmodules (gemm/conv + pointwise), MIGraphX checks whether the module can remain fused for a given rocMLIR tuning config viais_module_fusible(). If the config is not fusible, most notably whensplitKFactor > 1+ certain output fusions, the compiler falls back to splitting the module at a boundary computed byfind_final_split(). When trying to do this split MIGraphX could dereference an invalid iterator that would lead to a seg fault.Technical Details
find_final_split()could dereference an invalid iterator when:get_output_path()returned fewer than two instructionsstd::adjacent_find()found no valid split edge and returnedend().This is separate from #5144, which validates the pointwise submodule after a successful split. This PR fixes the crash that occurred during split-point selection.
Changelog Category
Add a
CHANGELOG.mdentry for any option other thanNot ApplicableFollow the LLVM AI Tool Use Policy for contributions using AI.