Skip to content

Conversation

@RealA10N
Copy link
Owner

@RealA10N RealA10N commented May 9, 2025

Fixes #51

@RealA10N RealA10N self-assigned this May 9, 2025
@RealA10N RealA10N linked an issue May 9, 2025 that may be closed by this pull request
@RealA10N RealA10N added the core A core feature or change label May 9, 2025
@RealA10N RealA10N marked this pull request as ready for review June 2, 2025 17:50
@codecov
Copy link

codecov bot commented Jun 2, 2025

Codecov Report

Attention: Patch coverage is 5.80645% with 876 lines in your changes missing coverage. Please review.

Project coverage is 37.15%. Comparing base (5be2b9c) to head (89dfd2d).
Report is 34 commits behind head on main.

Files with missing lines Patch % Lines
gen/assert.go 0.00% 82 Missing ⚠️
usm/isa/phi.go 0.00% 73 Missing ⚠️
gen/translate.go 0.00% 61 Missing ⚠️
usm/isa/ret.go 0.00% 54 Missing ⚠️
usm/isa/move.go 0.00% 48 Missing ⚠️
aarch64/isa/b.go 4.16% 46 Missing ⚠️
aarch64/isa/movz.go 4.25% 45 Missing ⚠️
usm/isa/call.go 0.00% 43 Missing ⚠️
aarch64/isa/bl.go 4.54% 42 Missing ⚠️
gen/file_info.go 0.00% 30 Missing ⚠️
... and 25 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #52      +/-   ##
==========================================
- Coverage   42.03%   37.15%   -4.89%     
==========================================
  Files         117      109       -8     
  Lines        4320     4481     +161     
==========================================
- Hits         1816     1665     -151     
- Misses       2418     2736     +318     
+ Partials       86       80       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@RealA10N RealA10N requested a review from Copilot June 2, 2025 17:51
Copy link
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 refactors instruction definition types across the gen and aarch64 packages to a new interface-based design, centralizes argument/target assertions, and consolidates validation and code generation logic.

  • Introduce Validate methods on FileInfo and BasicBlockInfo and replace inline assertion functions with shared helpers in gen/assert.go.
  • Convert all AArch64 ISA definitions to implement InstructionDefinition with Operator, Validate, and Codegen methods.
  • Update codegen and translation layers to invoke the new interfaces and adjust imports accordingly.

Reviewed Changes

Copilot reviewed 75 out of 75 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
gen/file_generator.go Use NewFileInfo + AppendFunction and validate early
gen/basic_block_info.go Add Validate method to BasicBlockInfo
gen/assert.go Centralize argument/target/integer assertions
aarch64/translation/targets.go Remove duplicate AssertTargetsExactly implementation
aarch64/translation/instructions.go Qualify assertions via gen import
aarch64/translation/arguments.go Remove inline assertion implementations
aarch64/managers/instructions.go Update instruction constructors to new names
aarch64/isa/*.go Migrate all ISA files to new functional InstructionDefinition design
aarch64/isa/add_test.go Update test helpers to return InstructionInfo + call Codegen
aarch64/codegen/instruction.go Switch from Generate to Codegen on the instruction interface
aarch64/codegen/file.go Adjust function index tracking for file codegen context
Comments suppressed due to low confidence (4)

aarch64/isa/add.go:63

  • Missing imports for "alon.kr/x/list" and "alon.kr/x/usm/core", which are required for list.FromSingle and core.Result.
return nil, list.FromSingle(core.Result{

aarch64/isa/subs.go:53

  • The code uses list.FromSingle and core.Result but neither "alon.kr/x/list" nor "alon.kr/x/usm/core" is imported; add these imports.
return nil, list.FromSingle(core.Result{

aarch64/isa/bcond.go:61

  • Missing imports for "alon.kr/x/list" and "alon.kr/x/usm/core" needed for list.FromSingle and core.Result in the default case.
return nil, list.FromSingle(core.Result{

aarch64/isa/b.go:90

  • list.FromSingle and core.Result are referenced without importing "alon.kr/x/list" and "alon.kr/x/usm/core"; please add those imports.
return nil, list.FromSingle(core.Result{

Co-authored-by: Copilot <[email protected]>
@RealA10N RealA10N merged commit 77177e1 into main Jun 2, 2025
3 checks passed
@RealA10N RealA10N deleted the 51-instruction-types-refactor branch June 2, 2025 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core A core feature or change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Merge InstructionDefinition and BaseInstruction into a single type

2 participants