Add arena IR SignatureConversion patterns#460
Merged
Conversation
Implement MLIR-style function signature conversion for the arena IR rewrite infrastructure, closing issue #438. - Add `set_block_arg_type` and `detach_region` to `IrContext` to support in-place block arg mutation and region transplanting - Thread `&ArenaTypeConverter` lifetime through `PatternRewriter<'a>` so patterns can call the type converter during rewrites - Update `ArenaRewritePattern::match_and_rewrite` to accept `PatternRewriter<'_>` (breaking change within crate, no public API impact) - Add `signature_conversion` module with: - `FuncSignatureConversionPattern`: converts `func.func` signatures - `WasmFuncSignatureConversionPattern`: converts `wasm.func` signatures Both patterns convert param/result types via the type converter, update entry block arg types in-place, and replace the op with a rebuilt function carrying the new signature and the original body. - 5 unit tests covering: i32→i64 conversion, no-change guard, wasm.func, effect attribute preservation, and partial (params-only) conversion Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds arena IR function signature-conversion rewrite patterns, threads an Changes
Sequence DiagramsequenceDiagram
participant Pattern as SignatureConversion Pattern
participant Converter as ArenaTypeConverter
participant Context as IrContext
participant Block as EntryBlock
participant Op as func/wasm.func
Pattern->>Op: match and extract func signature
Pattern->>Converter: convert return types
Converter-->>Pattern: converted return types
Pattern->>Converter: convert param types
Converter-->>Pattern: converted param types
Pattern->>Pattern: build ConvertedSignature
alt params changed
Pattern->>Block: request entry arg updates
Block->>Context: set_block_arg_type(block, index, new_ty)
Context-->>Block: BlockArgData + ValueData updated
end
Pattern->>Pattern: rebuild func.fn TypeRef
Pattern->>Context: detach_region(region)
Context-->>Pattern: region detached
Pattern->>Op: replace op with new type and reused body
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #460 +/- ##
=======================================
Coverage 78.54% 78.54%
=======================================
Files 10 10
Lines 4935 4935
=======================================
Hits 3876 3876
Misses 1059 1059
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/trunk-ir/src/arena/rewrite/signature_conversion.rs (1)
127-218: Consider extracting shared rewrite flow forfunc.funcandwasm.func.Both pattern implementations are structurally identical aside from dialect-specific wrappers/builders. A shared helper would reduce drift risk and keep future fixes synchronized.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/trunk-ir/src/arena/rewrite/signature_conversion.rs` around lines 127 - 218, Both match_and_rewrite implementations in FuncSignatureConversionPattern and WasmFuncSignatureConversionPattern duplicate the same rewrite flow; extract that flow into a shared helper (e.g., perform_signature_rewrite) that takes the OpRef plus small dialect-specific callbacks or a trait object for: validating/casting the op (the initial func::Func::from_op / wasm::Func::from_op), obtaining r#type, getting sym_name, getting body, and constructing the replacement op (currently func::func vs wasm::func). Inside the helper reuse convert_func_signature, update_entry_block_args, rebuild_func_type, ctx.detach_region, and rewriter.replace_op, then call this helper from each pattern's match_and_rewrite with the appropriate dialect lambdas so FuncSignatureConversionPattern and WasmFuncSignatureConversionPattern only provide dialect-specific pieces.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/trunk-ir/src/arena/rewrite/signature_conversion.rs`:
- Around line 127-218: Both match_and_rewrite implementations in
FuncSignatureConversionPattern and WasmFuncSignatureConversionPattern duplicate
the same rewrite flow; extract that flow into a shared helper (e.g.,
perform_signature_rewrite) that takes the OpRef plus small dialect-specific
callbacks or a trait object for: validating/casting the op (the initial
func::Func::from_op / wasm::Func::from_op), obtaining r#type, getting sym_name,
getting body, and constructing the replacement op (currently func::func vs
wasm::func). Inside the helper reuse convert_func_signature,
update_entry_block_args, rebuild_func_type, ctx.detach_region, and
rewriter.replace_op, then call this helper from each pattern's match_and_rewrite
with the appropriate dialect lambdas so FuncSignatureConversionPattern and
WasmFuncSignatureConversionPattern only provide dialect-specific pieces.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
crates/trunk-ir/src/arena/context.rscrates/trunk-ir/src/arena/rewrite/applicator.rscrates/trunk-ir/src/arena/rewrite/mod.rscrates/trunk-ir/src/arena/rewrite/pattern.rscrates/trunk-ir/src/arena/rewrite/rewriter.rscrates/trunk-ir/src/arena/rewrite/signature_conversion.rs
…re conversion `update_entry_block_args` previously updated block args in a partial loop when the entry block arg count differed from the new param count, leaving the IR in an inconsistent state. Change the function to return `bool` and bail out with `false` on any arity mismatch, leaving the IR untouched. Both `FuncSignatureConversionPattern` and `WasmFuncSignatureConversionPattern` now propagate that failure by returning `false` (no match). A new test `arity_mismatch_returns_unchanged` verifies that no changes are recorded and the original type is preserved when arities differ. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/trunk-ir/src/arena/rewrite/signature_conversion.rs (2)
132-223: Extract shared rewrite flow betweenfunc.funcandwasm.funcpatterns.Both
match_and_rewriteimplementations are nearly identical (convert signature, update entry args, detach body, rebuild op, replace). A shared helper would reduce drift risk and keep future safety fixes in one place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/trunk-ir/src/arena/rewrite/signature_conversion.rs` around lines 132 - 223, Extract the duplicated flow in FuncSignatureConversionPattern::match_and_rewrite and WasmFuncSignatureConversionPattern::match_and_rewrite into a single helper (e.g. rewrite_function_signature) that performs: call convert_func_signature, call update_entry_block_args when needed, call rebuild_func_type, detach the body region via ctx.detach_region, create the replacement op, and invoke rewriter.replace_op. The helper should accept the OpRef plus small accessors/constructors (closures or a trait) to get the op type, entry body, symbol name, and to construct the new op (so it can work for both func::Func and wasm::Func); then simplify both match_and_rewrite implementations to call this helper and return its boolean result, reusing existing functions convert_func_signature, update_entry_block_args, rebuild_func_type, ctx.detach_region, and rewriter.replace_op.
528-551: Consider adding a wasm arity-mismatch regression test for symmetry.You already lock this behavior for
func.func; mirroring it forwasm.funcwould guard against regressions in one pattern path only.➕ Suggested test addition
+ #[test] + fn wasm_arity_mismatch_returns_unchanged() { + let (mut ctx, loc) = test_ctx(); + let i32_ty = i32_type(&mut ctx); + let i64_ty = i64_type(&mut ctx); + + // Signature has 2 params, but entry block has only 1 arg (arity mismatch) + let func_ty = make_func_type(&mut ctx, &[i32_ty, i32_ty], i32_ty); + let func_op = make_wasm_func_op(&mut ctx, loc, "wasm_mismatched", func_ty, &[i32_ty]); + let module = make_module(&mut ctx, loc, vec![func_op]); + + let tc = i32_to_i64_converter(i32_ty, i64_ty); + let applicator = PatternApplicator::new(tc).add_pattern(WasmFuncSignatureConversionPattern); + let target = ArenaConversionTarget::new(); + + let result = applicator.apply(&mut ctx, module, &target).unwrap(); + assert_eq!(result.total_changes, 0); + + let ops = module.ops(&ctx); + let original_func = wasm::Func::from_op(&ctx, ops[0]).unwrap(); + assert_eq!(original_func.r#type(&ctx), func_ty); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/trunk-ir/src/arena/rewrite/signature_conversion.rs` around lines 528 - 551, Add a mirror test for wasm.func that follows arity_mismatch_returns_unchanged: create a wasm function type with two params but construct the wasm.func op with only one entry argument (arity mismatch), then apply the same i32_to_i64_converter + PatternApplicator but using the wasm signature conversion pattern (WasmFuncSignatureConversionPattern) and ArenaConversionTarget, assert result.total_changes == 0 and that the wasm.func's original type is preserved; mirror the existing test's setup symbols (i32_type, i64_type, make_func_type -> for wasm func type helper, make_wasm_func_op, make_module, i32_to_i64_converter, PatternApplicator, and ArenaConversionTarget) and name the test e.g. arity_mismatch_returns_unchanged_for_wasm.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/trunk-ir/src/arena/rewrite/signature_conversion.rs`:
- Around line 132-223: Extract the duplicated flow in
FuncSignatureConversionPattern::match_and_rewrite and
WasmFuncSignatureConversionPattern::match_and_rewrite into a single helper (e.g.
rewrite_function_signature) that performs: call convert_func_signature, call
update_entry_block_args when needed, call rebuild_func_type, detach the body
region via ctx.detach_region, create the replacement op, and invoke
rewriter.replace_op. The helper should accept the OpRef plus small
accessors/constructors (closures or a trait) to get the op type, entry body,
symbol name, and to construct the new op (so it can work for both func::Func and
wasm::Func); then simplify both match_and_rewrite implementations to call this
helper and return its boolean result, reusing existing functions
convert_func_signature, update_entry_block_args, rebuild_func_type,
ctx.detach_region, and rewriter.replace_op.
- Around line 528-551: Add a mirror test for wasm.func that follows
arity_mismatch_returns_unchanged: create a wasm function type with two params
but construct the wasm.func op with only one entry argument (arity mismatch),
then apply the same i32_to_i64_converter + PatternApplicator but using the wasm
signature conversion pattern (WasmFuncSignatureConversionPattern) and
ArenaConversionTarget, assert result.total_changes == 0 and that the wasm.func's
original type is preserved; mirror the existing test's setup symbols (i32_type,
i64_type, make_func_type -> for wasm func type helper, make_wasm_func_op,
make_module, i32_to_i64_converter, PatternApplicator, and ArenaConversionTarget)
and name the test e.g. arity_mismatch_returns_unchanged_for_wasm.
… wasm arity test Extract the duplicated match_and_rewrite logic from FuncSignatureConversionPattern and WasmFuncSignatureConversionPattern into a shared rewrite_function_signature helper that accepts a constructor closure. Also add a mirror arity mismatch test for wasm.func. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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
1 participant
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.
Summary
Completes the remaining work from #438 by porting
SignatureConversionpatterns to the arena IR rewrite infrastructure.set_block_arg_typeanddetach_regiontoIrContextto support in-place mutation of block argument types and region ownership transfertype_converter()accessor toPatternRewriter<'a>(with lifetime), propagated fromPatternApplicatorFuncSignatureConversionPatternandWasmFuncSignatureConversionPatternincrates/trunk-ir/src/arena/rewrite/signature_conversion.rsfunc.fntype signatures (params and return) viaArenaTypeConverter, update entry block argument types in-place, and rebuild the function op with the new signatureeffectattribute onfunc.fntypes across conversionTest plan
signature_conversion.rs:func_signature_i32_to_i64: full param + return conversion forfunc.funcno_change_when_types_not_matched: verifies the pattern is a no-op when no types matchwasm_func_signature_conversion: multi-param conversion forwasm.funceffect_attribute_preserved: verifies effect attribute survives conversionpartial_conversion_only_params: verifies partial conversion (params change, return already converted)cargo nextest run -p trunk-irto verify all tests passCloses #438
🤖 Generated with Claude Code
Summary by CodeRabbit