Add InstructionCollection merge functionality with register usage tracking#23
Conversation
…support Co-authored-by: petitstrawberry <14030185+petitstrawberry@users.noreply.github.com>
Co-authored-by: petitstrawberry <14030185+petitstrawberry@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
This PR implements InstructionCollection merge functionality to combine multiple instruction sequences from different builders in arbitrary order, specifically addressing the use case where register save/restore code is generated after determining which registers are used by the main computation.
- Adds basic merging operations (
append(),extend_from_collection(),concat()) with operator overloads (+,+=) forInstructionCollection - Introduces
InstructionCollectionWithUsagefor register-tracking enabled builds that merges both instructions and register usage statistics - Includes comprehensive test coverage and documentation with practical examples
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/common/mod.rs | Implements core merge functionality for InstructionCollection and InstructionCollectionWithUsage with tests |
| examples/instruction_collection_merge.rs | Complete example demonstrating prologue + main + epilogue merging use case |
| README.md | Documentation and usage examples for both basic and register-tracked merging |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| /// Create from raw parts. | ||
| pub fn from_parts(instructions: InstructionCollection<I>, register_usage: RegisterUsageInfo<R>) -> Self { | ||
| Self::new(instructions, register_usage) | ||
| } |
There was a problem hiding this comment.
The from_parts method is identical to new(). Consider removing this redundant method to reduce API surface area, or differentiate their purposes if they serve different use cases.
| /// Create from raw parts. | |
| pub fn from_parts(instructions: InstructionCollection<I>, register_usage: RegisterUsageInfo<R>) -> Self { | |
| Self::new(instructions, register_usage) | |
| } |
| self.instructions.append(other.instructions); | ||
| self.register_usage.merge(&other.register_usage); |
There was a problem hiding this comment.
This code pattern (append instructions + merge register usage) is duplicated in multiple methods (append, concat). Consider extracting this into a private helper method to reduce duplication.
| self.instructions.append(other.instructions); | ||
| self.register_usage.merge(&other.register_usage); | ||
| self | ||
| } |
There was a problem hiding this comment.
This method duplicates the same logic as the append method. Consider implementing concat by calling append internally: self.append(other); self.
| self.instructions.append(other.instructions); | |
| self.register_usage.merge(&other.register_usage); | |
| self | |
| } | |
| self.append(other); | |
| self | |
| } | |
| } |
| pub fn concat(mut self, mut other: InstructionCollection<I>) -> Self { | ||
| self.instructions.append(&mut other.instructions); |
There was a problem hiding this comment.
This method implementation is inconsistent with the tracked version. The tracked version takes ownership of other.instructions directly, while this version needs &mut other.instructions. Consider making the implementations consistent by removing the mut from the other parameter and using self.append(other) instead.
| pub fn concat(mut self, mut other: InstructionCollection<I>) -> Self { | |
| self.instructions.append(&mut other.instructions); | |
| pub fn concat(mut self, other: InstructionCollection<I>) -> Self { | |
| self.append(other); |
Overview
This PR implements the ability to merge
InstructionCollections from multiple builders in arbitrary order, addressing the use case where register save/restore code needs to be added before and after the main instruction sequence.Problem
When building complex functions (like the register save/restore example in the issue), the main processing code is often compiled first, and only after determining which registers are used can the prologue and epilogue be generated. However, the previous implementation only supported sequential instruction building within a single builder, making it impossible to combine pre-built instruction sequences in arbitrary order like
prologue + main + epilogue.Solution
1. Basic InstructionCollection Merging
Added merge operations to
InstructionCollection<I>:append()- Append instructions from another collection (consumes)extend_from_collection()- Extend with instructions from another collection (clones)concat()- Concatenate two collections (consumes both)+and+=for ergonomic merging2. Register Usage Tracking Integration
When the
register-trackingfeature is enabled, introducedInstructionCollectionWithUsage<I, R>that combines both instruction sequences and their register usage statistics. This allows proper merging of register tracking information across multiple code sequences:Testing
register-trackingfeatureDocumentation
examples/instruction_collection_merge.rsdemonstrating the exact use case from the issueBackward Compatibility
This PR is fully backward compatible:
Fixes #[issue_number]
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.