Lower reshape after eliminate_contiguous in a single pass - #5105
Open
ivarusic-amd wants to merge 13 commits into
Open
Lower reshape after eliminate_contiguous in a single pass#5105ivarusic-amd wants to merge 13 commits into
ivarusic-amd wants to merge 13 commits into
Conversation
Lower reshapes after contiguous elimination so layout decisions are validated locally and invalid lazy views fall back to safe copies. Co-authored-by: Cursor <cursoragent@cursor.com>
… a gpu::contiguous that reports the wrong shape
Collaborator
|
@ivarusic-amd what model are we seeing this in? Just message me in teams |
Regressions detected 🔴 * No develop baseline was found for this PR's branch point; compared against the latest available develop run instead. |
|
ivarusic-amd
changed the title
ivarusic-amd
force-pushed
the
wip/lower-reshape-review
branch
from
July 31, 2026 11:53
148a931 to
1ab2a09
Compare
…dentity permutation
Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Author
Affecting |
pfultz2
requested changes
Jul 31, 2026
|
|
||
| struct find_reshape : match::supports_dynamic_shapes | ||
| { | ||
| auto matcher() const { return match::name("reshape"); } |
Collaborator
There was a problem hiding this comment.
This should check there is only one argument: match::name("reshape")(match::nargs(1))
| // the wrong shape or fails in the copy itself. Reject it explicitly rather than | ||
| // lowering to something that silently computes the wrong result. | ||
| if(ins->inputs().size() == 2) | ||
| MIGRAPHX_THROW("lower_reshape: reshape with a runtime output buffer (2 input form) is " |
Collaborator
There was a problem hiding this comment.
Dont throw an error for this case. We could add a later pass to handle it.
| auto expected = ins->get_shape().to_symbolic(); | ||
| auto output_dims = ins->get_shape().sym_dims(); | ||
| auto reshaped = reshape_dims(s.to_symbolic(), output_dims, {.lazy = true}); | ||
| if(reshaped and *reshaped == expected) |
Collaborator
There was a problem hiding this comment.
This should use same_symbol with number elements:
Suggested change
| if(reshaped and *reshaped == expected) | |
| if(reshaped and same_symbol(reshaped->sym_elements(), expected.sym_elements()) |
|
|
||
| auto result = std::prev(m.end())->inputs().front(); | ||
| EXPECT(result->name() == "reshape_lazy"); | ||
| EXPECT(result->inputs().front()->name() == "gpu::contiguous"); |
Collaborator
There was a problem hiding this comment.
Tests should build the expected module.
| namespace gpu { | ||
|
|
||
| namespace { | ||
| instruction_ref insert_copy(module& m, |
Collaborator
There was a problem hiding this comment.
This should be named insert_precompile_op.
| instruction_ref pos, | ||
| instruction_ref input, | ||
| const operation& op, | ||
| const shape& output_shape) |
Collaborator
There was a problem hiding this comment.
Remove this parameter, this can be computed from the op and input.
| pos, make_op("gpu::precompile_op", {{"op", to_value(op)}}), input, alloc); | ||
| } | ||
|
|
||
| instruction_ref insert_standard_copy(module& m, instruction_ref pos, instruction_ref input) |
Collaborator
There was a problem hiding this comment.
This should be named insert_contiguous.
Comment on lines
+105
to
+108
| auto layout_op = make_op("layout", {{"permutation", perm}}); | ||
| auto layout_shape = layout_op.compute_shape({s}); | ||
| auto layout_reshape = reshape_dims(layout_shape, output_dims, {.lazy = true}); | ||
| if(layout_reshape and *layout_reshape == expected) |
Collaborator
There was a problem hiding this comment.
This should be removed. The reshape_dims or reshape_lazy should not fail here.
Suggested change
| auto layout_op = make_op("layout", {{"permutation", perm}}); | |
| auto layout_shape = layout_op.compute_shape({s}); | |
| auto layout_reshape = reshape_dims(layout_shape, output_dims, {.lazy = true}); | |
| if(layout_reshape and *layout_reshape == expected) |
Collaborator
|
I would like @shivadbhavsar to review this as well to make sure the symbolic things are correct. |
Contributor
Author
|
Comments will be addressed by wensday |
pfultz2
approved these changes
Aug 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
reshapelowering is currently split across three passes:lowering(add_reshape_lazy_op) expands everyreshapetogpu::contiguous -> reshape_lazy -> gpu::contiguous, unconditionally.eliminate_contiguousdeletes the redundant copies.propagate_reshape_layoutrewrites any surviving standardizing contiguous into alayoutto recover the discarded permutation.Step 1 runs before
eliminate_contiguous, so it cannot know the reshape's final inputlayout. Steps 2 and 3 exist to undo its guess.
Technical Details
Defer lowering until after
eliminate_contiguousand do it in one pass.add_reshape_lazy_opfromlowering.cpp;reshapepasses through untouched.propagate_reshape_layout(pass, header, test).lower_reshapein the same pipeline slot (target.cpp: aftereliminate_contiguous+dead_code_elimination, beforeadjust_allocation).Per
reshape, in order:reshape_dims(input, output_dims, {.lazy = true})succeeds and== expectedshape ->emit
reshape_lazyonly. Equality is required becausereshape_dimscan succeed withdifferent strides than
op::reshape::compute_shapeproduced.reshape_dims(output, input_dims, {.lazy = true})->find_permutation->layout; verify forward thatreshape_dims(layout_shape, output_dims, {.lazy = true}) == expected. Emitslayoutundergpu::precompile_op(one pointwise copy kernel viapointwise_compiler) +reshape_lazy. Forward check is required; the backwardsderivation is ambiguous for singleton dims.
gpu::contiguous+reshape_lazy.The trailing
gpu::contiguousis gone, so the result is no longer forced to standard;non-standard views now propagate further downstream than they did before.
Two-input
reshapenow throws. It lowered togpu::contiguous(data, out_buffer), butgpu::contiguous::compute_shapeusesinputs().at(0)only, soreshape(x{2,3,4}, out{6,4})reported{2,3,4}- andreplace_instructionalwaysrecomputes shape, so it is not overridable. No GPU copy op expresses a rank-changing copy;
all derive the kernel from one index space shared by src and dst, including
hip::copy(
"Ranks must be the same"dynamic,reorder_dims' rank assert static). Not a regression:developalso fails here via a 3-arggpu::contiguoustrippingcheck_shapes.has(2).simplify_dyn_opsalready folds compile-time-knowable targets into the one-input form.Changelog Category
Add a
CHANGELOG.mdentry for any option other thanNot Applicable