Skip to content

Add InstructionCollection merge functionality with register usage tracking#23

Merged
petitstrawberry merged 3 commits intodevelopfrom
copilot/fix-331ab7e2-fb6a-48e9-9807-c149ddbea76f
Oct 2, 2025
Merged

Add InstructionCollection merge functionality with register usage tracking#23
petitstrawberry merged 3 commits intodevelopfrom
copilot/fix-331ab7e2-fb6a-48e9-9807-c149ddbea76f

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Oct 2, 2025

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)
  • Operator overloading with + and += for ergonomic merging
let prologue = builder1.instructions();
let main_code = builder2.instructions();
let epilogue = builder3.instructions();

// Combine in any order
let combined = prologue + main_code + epilogue;

2. Register Usage Tracking Integration

When the register-tracking feature is enabled, introduced InstructionCollectionWithUsage<I, R> that combines both instruction sequences and their register usage statistics. This allows proper merging of register tracking information across multiple code sequences:

let prologue_tracked = InstructionCollectionWithUsage::new(
    prologue.instructions(),
    prologue.register_usage().clone()
);
let main_tracked = InstructionCollectionWithUsage::new(
    main_code.instructions(),
    main_code.register_usage().clone()
);
let epilogue_tracked = InstructionCollectionWithUsage::new(
    epilogue.instructions(),
    epilogue.register_usage().clone()
);

// Merge with register usage properly combined
let combined = prologue_tracked + main_tracked + epilogue_tracked;

Testing

  • Added 15 comprehensive tests covering:
    • Basic instruction merging operations
    • Empty collection edge cases
    • Multiple sequential merges
    • Register usage tracking integration
    • Operator overloading functionality
  • All 121 tests pass (106 existing + 15 new)
  • Tested with and without register-tracking feature

Documentation

  • Added complete example in examples/instruction_collection_merge.rs demonstrating the exact use case from the issue
  • Updated README.md with usage examples for both basic and register-tracked merging
  • Added inline documentation for all new public APIs

Backward Compatibility

This PR is fully backward compatible:

  • No changes to existing APIs
  • Only adds new functionality
  • Feature-gated register tracking support remains optional
  • All existing tests continue to pass

Fixes #[issue_number]

Original prompt

This section details on the original issue you should resolve

<issue_title>InstructionCollectionの結合機能</issue_title>
<issue_description>レジスタの退避、復元のように、ある処理中で使用されたレジスタが確定してからでないと命令が決定できないような場合、先にメインの処理をコンパイルしてから、その冒頭と末尾に退避、復元を追加する。

しかし、今の実装では一つのbuilderでは逐次的にしか命令を追加できないので、複数builderから生成されたInstructionCollectionを任意の順に結合できるような機能がほしい。

builder1 = メイン処理
builder2 = 退避
builder3 = 復元

それぞれの結果をinstCollection1,2,3とし、2+1+3のように結合したい。
この時、内部の使用済みレジスタなどの統計情報も適切にマージする。</issue_description>

Comments on the Issue (you are @copilot in this section)

Fixes #22

💡 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.

Copilot AI and others added 2 commits October 2, 2025 00:32
…support

Co-authored-by: petitstrawberry <14030185+petitstrawberry@users.noreply.github.com>
Co-authored-by: petitstrawberry <14030185+petitstrawberry@users.noreply.github.com>
Copilot AI changed the title [WIP] InstructionCollectionの結合機能 Add InstructionCollection merge functionality with register usage tracking Oct 2, 2025
Copilot AI requested a review from petitstrawberry October 2, 2025 00:36
Copy link
Copy Markdown
Owner

@petitstrawberry petitstrawberry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(๑•̀ㅂ•́)و✧

@petitstrawberry petitstrawberry marked this pull request as ready for review October 2, 2025 01:23
Copilot AI review requested due to automatic review settings October 2, 2025 01:23
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (+, +=) for InstructionCollection
  • Introduces InstructionCollectionWithUsage for 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.

Comment on lines +75 to +78
/// Create from raw parts.
pub fn from_parts(instructions: InstructionCollection<I>, register_usage: RegisterUsageInfo<R>) -> Self {
Self::new(instructions, register_usage)
}
Copy link

Copilot AI Oct 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
/// Create from raw parts.
pub fn from_parts(instructions: InstructionCollection<I>, register_usage: RegisterUsageInfo<R>) -> Self {
Self::new(instructions, register_usage)
}

Copilot uses AI. Check for mistakes.
Comment on lines +114 to +115
self.instructions.append(other.instructions);
self.register_usage.merge(&other.register_usage);
Copy link

Copilot AI Oct 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment on lines +128 to +131
self.instructions.append(other.instructions);
self.register_usage.merge(&other.register_usage);
self
}
Copy link

Copilot AI Oct 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method duplicates the same logic as the append method. Consider implementing concat by calling append internally: self.append(other); self.

Suggested change
self.instructions.append(other.instructions);
self.register_usage.merge(&other.register_usage);
self
}
self.append(other);
self
}
}

Copilot uses AI. Check for mistakes.
Comment on lines +490 to +491
pub fn concat(mut self, mut other: InstructionCollection<I>) -> Self {
self.instructions.append(&mut other.instructions);
Copy link

Copilot AI Oct 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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);

Copilot uses AI. Check for mistakes.
@petitstrawberry petitstrawberry merged commit 614659b into develop Oct 2, 2025
6 checks passed
@petitstrawberry petitstrawberry deleted the copilot/fix-331ab7e2-fb6a-48e9-9807-c149ddbea76f branch October 2, 2025 02:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

InstructionCollectionの結合機能

3 participants