fix(pass): Repair matrix layout for row-major ops#1231
fix(pass): Repair matrix layout for row-major ops#1231lwDavid wants to merge 1 commit intohw-native-sys:mainfrom
Conversation
Fixes hw-native-sys#1229 ResolveBackendOpLayouts now inserts same-memory tile.move layout repairs for non-row-major matrix tiles before backend ops that require row_major inputs, while preserving the existing column-vector reshape path. Restores the original result layout after constrained ops and adds regression coverage for tile.exp on a col_major matrix tile.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe Changes
Sequence DiagramsequenceDiagram
participant Pass as ResolveBackendOpLayouts
participant Check as Layout Checker
participant Repair as Repair Logic
participant IR as IR Builder
Pass->>Check: Check NeedsInputRepair for tile
Check-->>Pass: True if input needs row_major
Pass->>Repair: Determine repair method
alt Column Vector + col_major
Repair->>IR: Insert tile.reshape(arg, [1,N])
else General Matrix + non-row_major
Repair->>IR: Insert tile.move with blayout=row_major
end
IR-->>Pass: Repaired operand
Pass->>Pass: Execute backend operation
Pass->>Check: Check output restoration needs
Check-->>Pass: Determine restoration method
alt Column Vector Result
Repair->>IR: Insert tile.reshape back
else General Matrix Result
Repair->>IR: Insert tile.move back to original blayout
end
IR-->>Pass: Restored tile
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
There was a problem hiding this comment.
Code Review
This pull request enhances the ResolveBackendOpLayouts pass to support general matrix layout coercion. Previously, the pass only handled [N, 1] column-major vectors by reshaping them into row-major views; it now utilizes tile.move to coerce arbitrary non-row-major tiles into the required row_major layout and ensures the original layout is restored for assignment results. The review feedback identifies opportunities to optimize the C++ implementation by avoiding unnecessary heap allocations caused by temporary TileView object constructions during layout retrieval and restoration.
| return TileLayout::row_major; | ||
| } | ||
| return tile_type->tile_view_->blayout; | ||
| return tile_type->tile_view_.value_or(TileView{}).blayout; |
There was a problem hiding this comment.
Using value_or(TileView{}) creates a temporary TileView object, which includes a std::vector for valid_shape. This can lead to unnecessary heap allocations. Since a tile without an explicit view is implicitly row-major in this project, it is more efficient to use a ternary operator or an explicit check to avoid constructing the temporary.
| return tile_type->tile_view_.value_or(TileView{}).blayout; | |
| return tile_type->tile_view_ ? tile_type->tile_view_->blayout : TileLayout::row_major; |
| if (IsColumnVector(result_tile_type)) { | ||
| restore_call = CreateReshapeCall(row_major_var, result_tile_type->shape_, call->span_); | ||
| } else { | ||
| auto target_view = result_tile_type->tile_view_.value_or(TileView{}); |
There was a problem hiding this comment.
Similar to the efficiency concern in GetTileLayout, using value_or(TileView{}) here creates a temporary object. Since this code path is only reached if result_tile_type is not row-major (which implies it must have a tile_view_ with a non-row-major layout), you can safely use .value() to access the existing view and avoid the overhead of constructing a default TileView.
| auto target_view = result_tile_type->tile_view_.value_or(TileView{}); | |
| const auto& target_view = result_tile_type->tile_view_.value(); |
Summary
Fixes #1229.
ResolveBackendOpLayoutsnow repairs general non-row-major matrix tiles before backend ops that requirerow_majorlayout. Previously the pass only handled[N, 1]column-vector reshape repair, so full matrix tiles produced by paths such asmatmul/tpop -> Vec -> neg -> expcould still reachpto.texpwithblayout=col_major, causingptoasto fail.Root Cause
The backend layout spec for ops like
tile.exprequires row-major inputs and outputs. The old repair pass only rewrote[N, 1]col-major vectors into[1, N]row-major views viatile.reshape. It skipped general matrix tiles such as[16, 256]withblayout=col_major, slayout=row_major, so the generated PTO kept a non-row-major source forpto.texp.Changes
ResolveBackendOpLayoutsto detect any constrained tile input/output that is not row-major.[N, 1]vector reshape fast path.tile.move(..., blayout=row_major, slayout=none_box)for general non-row-major matrix inputs.tile.reshapefor column vectors ortile.movefor general matrix tiles.tile.expon a col-major matrix tile.