Unify register references into single References field#67
Open
Unify register references into single References field#67
Conversation
Replace the concrete TargetInfo struct with the existing ArgumentInfo interface for instruction targets. RegisterArgumentInfo is reused for register targets, with AppendTarget doing a type assertion to call AddDefinition (analogous to how argument generators call AddUsage directly). This decouples the gen layer from the assumption that targets must always be registers, enabling future ISA-defined target kinds such as variables (stack locals, see issue #45). - Delete gen/target_info.go; TargetInfo replaced by ArgumentInfo - InstructionInfo.Targets: []*TargetInfo → []ArgumentInfo - AppendTarget accepts ...ArgumentInfo; type-asserts *RegisterArgumentInfo to call Register.AddDefinition (removes SwitchTarget) - TargetGenerator returns ArgumentInfo, creates *RegisterArgumentInfo - TargetsToRegisters / TargetToType updated to accept ArgumentInfo - SSA renameTarget type-asserts *RegisterArgumentInfo for renaming - AArch64 translation helpers accept gen.ArgumentInfo https://claude.ai/code/session_019Ze4A35Lngx3UQvvdbkD3V
…4GP*
Since targets are now ArgumentInfo (same type as arguments),
TargetToAarch64GPRegister and TargetToAarch64GPorSPRegister were
identical to their Argument* counterparts. Delete targets.go and
update all callers to use ArgumentToAarch64GP{Register,orSPRegister}
directly on info.Targets[i].
https://claude.ai/code/session_019Ze4A35Lngx3UQvvdbkD3V
ArgumentInfo.String() now includes the type prefix for registers (e.g. "$32 %a" instead of "%a"), consistent with ImmediateInfo which already does this. This lets InstructionInfo.String() drop the RegisterArgumentInfo type-assertion in the target loop and simply call target.String() for all targets. ArgumentParser gains a TypeParser and tries $type %name with backtracking before falling back to bare %name, so source files with or without explicit type annotations on register arguments both parse correctly. Test src strings updated to match the new canonical format. https://claude.ai/code/session_019Ze4A35Lngx3UQvvdbkD3V
Since type prefixes on register arguments are optional, the test source strings don't need to use the typed form. Parse from untyped source; compare String() output against a separate typed expected string. https://claude.ai/code/session_019Ze4A35Lngx3UQvvdbkD3V
Instead of saving/restoring the TokenView in ArgumentParser, introduce AnnotatedRegisterParser which uses two-token lookahead: if token 0 is a type and token 1 is a register, consume the type annotation (and discard it) before parsing the register normally. ArgumentParser just uses NewAnnotatedRegisterParser() as its RegisterParser. https://claude.ai/code/session_019Ze4A35Lngx3UQvvdbkD3V
…helpers - AnnotatedRegisterParser is gone: RegisterParser now embeds TypeParser and overrides Parse with the two-token lookahead itself. ArgumentParser goes back to NewRegisterParser(). - TargetsToRegisters deleted: it was identical to ArgumentsToRegisters now that targets are []ArgumentInfo. Three callers updated. - TargetToType deleted: it was a silent-failure variant of ArgumentToType. Three callers (binary_calculation, move, phi) updated to use ArgumentToType with proper error propagation. https://claude.ai/code/session_019Ze4A35Lngx3UQvvdbkD3V
Move already embeds DefinesTargetsInstruction and UsesArgumentsInstruction which provide identical implementations. The explicit methods were left over from an earlier refactor. https://claude.ai/code/session_019Ze4A35Lngx3UQvvdbkD3V
…Info RegisterInfo now has a single References []*InstructionInfo that tracks every instruction referencing the register, regardless of whether it appears as a target or an argument. An instruction appears once per slot (e.g. "add %x %x" adds two entries), matching the previous per-list behaviour. Key changes: - gen/register_info.go: Definitions + Usages → References; AddDefinition/RemoveDefinition/AddUsage/RemoveUsage → AddReference/RemoveReference. - gen/register_argument.go: OnAttach/OnDetach/SwitchRegister use AddReference/RemoveReference; manual AddUsage removed from generator. - gen/instruction_info.go: AppendTarget calls target.OnAttach (no more RegisterArgumentInfo type assertion); AppendArgument calls argument.OnAttach (reference tracking no longer split across generator and append). - opt/dead_code_elimination.go: traces from uses to definitions by filtering register.References with the DCESupportedInstruction.Defines() trait instead of relying on a pre-computed Definitions field. - opt/ssa/construction_scheme.go: IsDefinition added to SsaConstructionScheme so phi-insertion can filter References without generic code knowing about register/target specifics. - opt/ssa/function_ssa_info.go: getDefinitions uses IsDefinition. - usm/ssa/ssa_construction.go: implements IsDefinition; renameTarget simplified to use SwitchRegister (same as renameArgument). https://claude.ai/code/session_019Ze4A35Lngx3UQvvdbkD3V
Replace IsDefinition on SsaConstructionScheme with a per-instruction SSASupportedInstruction interface (analogous to DCESupportedInstruction). getDefinitions now filters register.References by type-asserting each instruction's definition to SSASupportedInstruction and checking Defines(). Any instruction already implementing DCESupportedInstruction.Defines() satisfies the new interface automatically, so no ISA changes are needed. Also add TokenView.PeekTokens, which peeks at N consecutive token types without consuming, and use it in RegisterParser to simplify the optional type-prefix lookahead from nested ifs to a single condition. https://claude.ai/code/session_019Ze4A35Lngx3UQvvdbkD3V
for more information, see https://pre-commit.ci
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #67 +/- ##
==========================================
- Coverage 41.00% 40.90% -0.11%
==========================================
Files 123 119 -4
Lines 3790 3819 +29
==========================================
+ Hits 1554 1562 +8
- Misses 2134 2149 +15
- Partials 102 108 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…er loop The continue was masking a potential internal inconsistency: if a register's reference instruction doesn't implement DCESupportedInstruction, DCE could silently treat that register as having no definition and produce broken IR. The outer collectCriticalInstructions pass already enforces the interface on all instructions and errors early, so hitting the inner branch is always a bug — treat it as an internal error consistently. https://claude.ai/code/session_019Ze4A35Lngx3UQvvdbkD3V
…efinitions The SSASupportedInstruction interface was "optional", meaning a missing implementation was silently skipped via continue. But targets add themselves to register.References via OnAttach, so getDefinitions already iterates definition sites — it just needs to check instruction.Targets directly instead of delegating to an optional interface. This approach is strictly safer: no interface can be forgotten, no definition site can be silently missed, and the SSASupportedInstruction type is removed entirely since it is no longer needed. https://claude.ai/code/session_019Ze4A35Lngx3UQvvdbkD3V
The previous commit incorrectly replaced SSASupportedInstruction.Defines() with direct target inspection, assuming targets == definitions. That is not guaranteed — an instruction may define registers not listed as targets (e.g. implicit definitions in future ISA backends). Restore the interface (now documented as required, not optional) and make getDefinitions return an internal error when an instruction does not implement it, consistent with how DCE handles the same situation. Propagate the error through getRegisterPhiInsertionPoints up to InsertPhiInstructions. https://claude.ai/code/session_019Ze4A35Lngx3UQvvdbkD3V
RealA10N
commented
Mar 27, 2026
Comment on lines
37
to
+50
| func TestSimpleFunctionGeneration(t *testing.T) { | ||
| src := `func $32 @add $32 %a { | ||
| .entry | ||
| $32 %b = add %a $32 #1 | ||
| $32 %c = add %b %a | ||
| ret | ||
| } | ||
| ` | ||
| expected := `func $32 @add $32 %a { | ||
| .entry | ||
| $32 %b = add $32 %a $32 #1 | ||
| $32 %c = add $32 %b $32 %a | ||
| ret | ||
| } |
Owner
Author
There was a problem hiding this comment.
Previously, a register must have been declared explicitly as a target with an explict type at least once in a function. What is the behavior now?
Ideally, I want to still derive register types by not requiring explicit register types anywhere, however I think that we might be able to lift the requirement for the target to be explicit and just make it so a register must appear at least once with an explicit type inside every function: it can be as a argument or as a target.
RealA10N
commented
Mar 27, 2026
| } | ||
| return core.ResultList{} | ||
| } | ||
|
|
Owner
Author
There was a problem hiding this comment.
IIUC this assumes that assignments to registers (before converting to SSA form) is done only to targets, because here we rename only "targets". I now want to support that the ISA itself might define the syntax, and thus register assignments might be done to arguments to the instruction and not necessary targets only. Am I right with my understanding? If so, think of a way to support this behavior. Perhaps, we need to define new interfaces / methods to the SSASupportedInstruction interface.
**Comment 1** (function_generator_test.go:50): Allow register type derivation from typed arguments, not just targets. - parse/register.go: Add optional Type *TypeNode to RegisterNode. RegisterParser.Parse now preserves the type annotation instead of discarding it, so "$32 %a" in argument position carries the type. - gen/function_generator.go: Extend collectRegisterDefinitions to also scan instruction arguments for typed register references. For each typed register argument a synthetic TargetNode is fed to TargetGenerator, reusing the same create-or-validate-type logic. - gen/register_generator.go: Update error hint to reflect that an explicit type can appear in either a target or a typed argument. - parse/*_test.go: Fix positional RegisterNode struct literals (field names required now that RegisterNode has two fields). **Comment 2** (ssa_construction.go:72): SSA register renaming should not assume definitions live only in Targets; ISAs may define registers via instruction arguments. - opt/ssa/construction_scheme.go: Add DefinitionArguments to SSASupportedInstruction. The method returns the *RegisterArgumentInfo objects that an instruction writes, wherever they live (Targets, Arguments, or elsewhere). - opt/dead_code_elimination.go: Implement DefinitionArguments on DefinesTargetsInstruction (extract RegisterArgumentInfo from Targets) and DefinesNothingInstruction (returns empty slice), so existing instructions continue to satisfy SSASupportedInstruction. - usm/ssa/ssa_construction.go: Replace the Targets iteration in renameInstruction with a call to SSASupportedInstruction .DefinitionArguments, and add newSSANotSupportedError for when an instruction does not implement the interface. https://claude.ai/code/session_019Ze4A35Lngx3UQvvdbkD3V
RealA10N
commented
Mar 27, 2026
- opt/ssa/construction_scheme.go, opt/dead_code_elimination.go, usm/ssa/ssa_construction.go: Add UsesArguments to SSASupportedInstruction (parallel to DefinitionArguments). renameInstruction now delegates to ssaInstr.UsesArguments / ssaInstr.DefinitionArguments for both use- and definition-renaming, removing the silent *RegisterArgumentInfo type assertion in the former renameArgument helper. UsesArgumentsInstruction and UsesNothingInstruction in the opt package implement the new method. - usm/isa/phi.go: Collect the ArgumentToType error for the target alongside the target-count and even-arguments errors, behind a single early return, so all structural errors are reported together. - gen/register_generator.go: Shorten the hint message as requested. https://claude.ai/code/session_019Ze4A35Lngx3UQvvdbkD3V
RealA10N
commented
Mar 27, 2026
| Type: regNode.Type, | ||
| Register: parse.RegisterNode{TokenNode: regNode.TokenNode}, | ||
| } | ||
| _, curResults := g.TargetGenerator.Generate(ctx, syntheticTarget) |
Owner
Author
There was a problem hiding this comment.
Maybe the name TargetGenerator doesn't do justice anymore. Think of a better name, and perhaps refine the implementation so we don't need to "hack it" with synthetic target types.
Coverage fixes: - parse/instruction_test.go: Add TestInstructionWithTypedRegisterArgument to cover RegisterNode.Type being populated when a type annotation precedes a register in argument position. - parse/register.go: Add RegisterNode.String() that includes the type prefix when Type != nil (required for the test's string assertion). - gen/function_generator_test.go: Add TestRegisterTypeFromArgument to cover the new collectRegisterDefinitions branch that derives a register's type from a typed argument rather than a typed target. Review comment (function_generator.go:129): - Rename TargetGenerator → TypedRegisterGenerator throughout (gen/target_generator.go, gen/instruction_generator.go, gen/function_generator.go, gen/target_generator_test.go). The generator handles any typed register declaration — not only syntactic targets — so the old name no longer reflected its scope. https://claude.ai/code/session_019Ze4A35Lngx3UQvvdbkD3V
… ArgumentGenerator/TargetGenerator
- Rename the type TypedRegisterGenerator to RegisterDeclarationGenerator in
gen/target_generator.go (ArgumentGenerator was already taken by the
instruction-argument dispatcher in argument_generator.go)
- Rename the field TypedRegisterGenerator in FunctionGenerator to ArgumentGenerator
(as suggested in review) and in InstructionGenerator to TargetGenerator
- Fix register name lookup in RegisterDeclarationGenerator and RegisterGenerator
to use node.TokenNode (just the register token) instead of node.View() which
now includes the optional type annotation prefix
- Update test struct literals for TargetNode = RegisterNode alias: replace
{Type: ..., Register: RegisterNode{TokenNode: ...}} with {TokenNode: ..., Type: ...}
https://claude.ai/code/session_019Ze4A35Lngx3UQvvdbkD3V
- parse/instruction.go: change InstructionNode.Targets from []TargetNode (= []RegisterNode) to []ArgumentNode; InstructionParser uses a plain RegisterParser loop instead of a TargetParser field so that the syntactic restriction (registers only) is preserved while the AST type is the general ArgumentNode interface - parse/target.go: deleted — TargetNode alias and NewTargetParser were fully subsumed by RegisterNode / NewRegisterParser - gen/instruction_generator.go: remove dedicated TargetGenerator field; generateTargets now uses the existing ArgumentGenerator, which dispatches through RegisterArgumentGenerator to look up the already-declared registers (declaration is a pre-pass responsibility) - gen/function_generator.go: unify the two collectRegisterDefinitions loops (targets + typed arguments) into a single loop over the combined slice, since both are now []ArgumentNode - tests updated throughout for the type change https://claude.ai/code/session_019Ze4A35Lngx3UQvvdbkD3V
The six methods added to DCE helper structs (UsesArgumentsInstruction, UsesNothingInstruction, DefinesTargetsInstruction, DefinesNothingInstruction) for the SSASupportedInstruction interface were not exercised by existing DCE tests, dropping opt package coverage from 79% to 66%. Add direct unit tests covering all six methods, restoring opt coverage and bringing total project coverage above main. https://claude.ai/code/session_019Ze4A35Lngx3UQvvdbkD3V
…Generator - Delete gen/target_generator.go (RegisterDeclarationGenerator, NewRegisterDeclarationGenerator, NewRegisterTypeMismatchResult) - Inline register declaration logic into FunctionGenerator.declareRegister(), using the already-present ReferencedTypeGenerator instead of a separate generator - Move NewRegisterTypeMismatchResult to register_generator.go - Update collectRegisterDefinitions to call g.declareRegister() directly - Delete gen/target_generator_test.go (covered by function_generator_test.go) - Update TestInstructionCreateTarget to manually pre-populate registers instead of relying on the now-deleted RegisterDeclarationGenerator https://claude.ai/code/session_019Ze4A35Lngx3UQvvdbkD3V
…ntParser for targets - Remove separate Uses/Defines methods from DCESupportedInstruction and SSASupportedInstruction; both interfaces now use UsesArguments/DefinitionArguments returning []*RegisterArgumentInfo, which subsumes the register-only variants - Update DCE collectUsefulInstructions and SSA getDefinitions to extract .Register from argument slots instead of calling the now-deleted methods - Remove UsesArgumentsInstruction.Uses, UsesNothingInstruction.Uses, DefinesTargetsInstruction.Defines, DefinesNothingInstruction.Defines - Delete gen.ArgumentsToRegisters (no longer needed) - Remove redundant Call.Defines/Call.Uses overrides (embedded mixins suffice) - InstructionParser: replace RegisterParser-based target loop with ArgumentParser + backtracking (save/restore TokenView); targets are now any argument sequence followed by '=', not just registers - Update grammar comment: Lbl* (Arg+ Eql)? Opr? Arg* \n+ - Remove InstructionParser.RegisterParser field and parseEquals helper https://claude.ai/code/session_019Ze4A35Lngx3UQvvdbkD3V
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
2 participants
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
This PR consolidates the separate
DefinitionsandUsagesfields inRegisterInfointo a singleReferencesfield that tracks all instruction references to a register, whether as a target or argument. This simplifies the data structure and makes the codebase more maintainable.Key Changes
RegisterInfo refactoring: Replaced
DefinitionsandUsagesfields with a unifiedReferencesfield that tracks all instruction references (both definitions and uses)AddDefinition/RemoveDefinitionandAddUsage/RemoveUsagemethods toAddReference/RemoveReferenceTargetInfo removal: Deleted the
TargetInfostruct and converted targets to useArgumentInfointerfaceRegisterArgumentInfo(which implementsArgumentInfo) instead ofTargetInfoInstructionInfo.Targetsfrom[]*TargetInfoto[]ArgumentInfoTargetToAarch64GPRegisterandTargetToAarch64GPorSPRegisterhelper functionsArgumentInfo interface expansion: Enhanced
ArgumentInfoto support both arguments and targetsOnAttachandOnDetachmethods to handle register reference trackingRegisterArgumentInfonow implements these methods to update register referencesParser improvements: Enhanced
RegisterParserto optionally consume type annotations%nameand$type %namesyntax in argument positionsHelper function updates:
TargetsToRegisterstoArgumentsToRegistersfor consistencyArgumentToTypehelper function to extract type fromArgumentInfoTargetToTypefunctionInstruction generation: Updated to use the new unified reference tracking
AppendTargetandAppendArgumentnow callOnAttachon argumentsSwitchTargetmethod in favor of usingRegisterArgumentInfo.SwitchRegisterSSA and optimization updates: Updated dead code elimination and SSA construction to work with unified references
Referencesby checking if instructions actually define the registerSSASupportedInstructioninterface for phi insertionTest updates: Updated test expectations to reflect the new unified reference tracking
https://claude.ai/code/session_019Ze4A35Lngx3UQvvdbkD3V