From 89bd30bfbddd9999cb5f83e4d2981a583f16463c Mon Sep 17 00:00:00 2001 From: Mattias Hansson Date: Wed, 3 Jun 2026 23:23:54 +0200 Subject: [PATCH] Fixed optimizer bugs --- internal/compiler/compiler.go | 12 +++++- internal/optimizer/config.go | 22 +++++----- internal/optimizer/config_test.go | 54 ++++++++++++++++++++++++ internal/optimizer/debug.go | 62 +++++++++++++++++++++++++--- internal/optimizer/optimizer.go | 13 +++--- internal/optimizer/optimizer_test.go | 43 +++++++++++++++++++ internal/optimizer/pass_imm.go | 52 +++++++++++++---------- internal/optimizer/pass_jmp.go | 4 -- internal/optimizer/pass_load.go | 34 +++++++++++---- internal/optimizer/pass_self.go | 4 -- internal/optimizer/regstate.go | 18 ++++++-- internal/optimizer/scanner.go | 18 ++++++-- 12 files changed, 268 insertions(+), 68 deletions(-) create mode 100644 internal/optimizer/config_test.go diff --git a/internal/compiler/compiler.go b/internal/compiler/compiler.go index d92b324..40efdc2 100644 --- a/internal/compiler/compiler.go +++ b/internal/compiler/compiler.go @@ -234,7 +234,7 @@ func (c *Compiler) Compile(lines []preproc.Line) ([]string, error) { } codeOutput = append(codeOutput, fmt.Sprintf("; %s", line.Text)) - if len(asmLines) > 0 && c.isOptimizing() { + if len(asmLines) > 0 && c.isMarkersEnabled() { codeOutput = append(codeOutput, fmt.Sprintf("; @@OPT:%s:%s", classString(cmd.GetClass()), cmd.GetName())) } codeOutput = append(codeOutput, asmLines...) @@ -296,6 +296,16 @@ func (c *Compiler) isOptimizing() bool { return cfg.Any() } +// isMarkersEnabled returns true if _P_OPT_MARKERS is active +func (c *Compiler) isMarkersEnabled() bool { + if c.ctx == nil || c.ctx.Pragma == nil { + return false + } + idx := c.ctx.Pragma.GetCurrentPragmaSetIndex() + ps := c.ctx.Pragma.GetPragmaSetByIndex(idx) + return ps.GetPragma("_P_OPT_MARKERS") != "" && ps.GetPragma("_P_OPT_MARKERS") != "0" +} + // getOptimizerConfig returns optimizer config if optimization is enabled, nil otherwise func (c *Compiler) getOptimizerConfig() *optimizer.Config { if c.ctx == nil || c.ctx.Pragma == nil { diff --git a/internal/optimizer/config.go b/internal/optimizer/config.go index 49cf2e6..a6d7ef0 100644 --- a/internal/optimizer/config.go +++ b/internal/optimizer/config.go @@ -5,22 +5,24 @@ import ( ) type Config struct { - EnableLoad bool - EnableImm bool - EnableJmp bool - EnableSelf bool - Debug bool + EnableLoad bool + EnableImm bool + EnableJmp bool + EnableSelf bool + Debug bool + ShowMarkers bool } func NewConfig(ps preproc.PragmaSet) *Config { all := ps.GetPragma("_P_OPT_ALL") != "" && ps.GetPragma("_P_OPT_ALL") != "0" return &Config{ - EnableLoad: all || (ps.GetPragma("_P_OPT_LOAD") != "" && ps.GetPragma("_P_OPT_LOAD") != "0"), - EnableImm: all || (ps.GetPragma("_P_OPT_IMM") != "" && ps.GetPragma("_P_OPT_IMM") != "0"), - EnableJmp: all || (ps.GetPragma("_P_OPT_JMP") != "" && ps.GetPragma("_P_OPT_JMP") != "0"), - EnableSelf: all || (ps.GetPragma("_P_OPT_SELF") != "" && ps.GetPragma("_P_OPT_SELF") != "0"), - Debug: (ps.GetPragma("_P_OPT_DEBUG") != "" && ps.GetPragma("_P_OPT_DEBUG") != "0"), + EnableLoad: all || (ps.GetPragma("_P_OPT_LOAD") != "" && ps.GetPragma("_P_OPT_LOAD") != "0"), + EnableImm: all || (ps.GetPragma("_P_OPT_IMM") != "" && ps.GetPragma("_P_OPT_IMM") != "0"), + EnableJmp: all || (ps.GetPragma("_P_OPT_JMP") != "" && ps.GetPragma("_P_OPT_JMP") != "0"), + EnableSelf: all || (ps.GetPragma("_P_OPT_SELF") != "" && ps.GetPragma("_P_OPT_SELF") != "0"), + Debug: (ps.GetPragma("_P_OPT_DEBUG") != "" && ps.GetPragma("_P_OPT_DEBUG") != "0"), + ShowMarkers: (ps.GetPragma("_P_OPT_MARKERS") != "" && ps.GetPragma("_P_OPT_MARKERS") != "0"), } } diff --git a/internal/optimizer/config_test.go b/internal/optimizer/config_test.go new file mode 100644 index 0000000..adcbbe7 --- /dev/null +++ b/internal/optimizer/config_test.go @@ -0,0 +1,54 @@ +package optimizer + +import ( + "testing" + "c65gm/internal/preproc" +) + +func TestNewConfigWithAll(t *testing.T) { + ps := preproc.NewPragmaSet(map[string]string{ + "_P_OPT_ALL": "1", + "_P_OPT_DEBUG": "1", + }) + cfg := NewConfig(ps) + if !cfg.Any() { + t.Error("expected Any()=true") + } + if !cfg.EnableLoad { + t.Error("expected EnableLoad=true from _P_OPT_ALL") + } + if !cfg.EnableImm { + t.Error("expected EnableImm=true from _P_OPT_ALL") + } + if !cfg.Debug { + t.Error("expected Debug=true") + } +} + +func TestNewConfigPragmasAccumulate(t *testing.T) { + // Pragma sets COPY previous values — so last set has ALL pragmas + p := preproc.NewPragma() + p.AddPragma("_P_OPT_ALL", "1") + p.AddPragma("_P_OPT_DEBUG", "1") + p.AddPragma("_P_SOMETHING_ELSE", "1") + + // Last set should have all three + ps := p.GetPragmaSetByIndex(p.GetCurrentPragmaSetIndex()) + if v := ps.GetPragma("_P_OPT_ALL"); v != "1" { + t.Errorf("expected _P_OPT_ALL=1, got %q", v) + } + if v := ps.GetPragma("_P_OPT_DEBUG"); v != "1" { + t.Errorf("expected _P_OPT_DEBUG=1, got %q", v) + } + if v := ps.GetPragma("_P_SOMETHING_ELSE"); v != "1" { + t.Errorf("expected _P_SOMETHING_ELSE=1, got %q", v) + } + + cfg := NewConfig(ps) + if !cfg.Any() { + t.Error("expected Any()=true") + } + if !cfg.Debug { + t.Error("expected Debug=true") + } +} diff --git a/internal/optimizer/debug.go b/internal/optimizer/debug.go index 629e1e8..378e4e5 100644 --- a/internal/optimizer/debug.go +++ b/internal/optimizer/debug.go @@ -2,10 +2,62 @@ package optimizer import "fmt" -// debugWithOriginals wraps the current state with a debug comment header -func debugWithOriginals(lines []asmLine, phase string) []asmLine { - header := fmt.Sprintf("; --- %s ---", phase) - result := []asmLine{{text: header, isComment: true}} - result = append(result, lines...) +// debugDiff produces annotated output showing original vs optimized code. +// Lines that were removed are shown with "[removed]" annotation. +func debugDiff(original, optimized []asmLine) []asmLine { + origCount := countCodeLines(original) + optCount := countCodeLines(optimized) + saved := origCount - optCount + + header := fmt.Sprintf("; --- peephole: %d asm lines → %d (%d saved) ---", origCount, optCount, saved) + var result []asmLine + result = append(result, asmLine{text: header, isComment: true}) + + // Walk both arrays in sync. When optimized doesn't have a line + // that original has, it was removed. + oi := 0 // index into original (pre-stripped) + opi := 0 // index into optimized + + for oi < len(original) { + origLine := original[oi] + + if opi < len(optimized) && linesMatch(origLine, optimized[opi]) { + result = append(result, optimized[opi]) + oi++ + opi++ + } else if !origLine.isCode { + // Non-code line that doesn't match anything — pass through + result = append(result, origLine) + oi++ + } else { + // Code line in original but not in optimized → removed + result = append(result, asmLine{ + text: fmt.Sprintf("; [removed] %s", origLine.text), + isComment: true, + }) + oi++ + } + } + + // Append any remaining optimized lines (shouldn't happen in practice) + for ; opi < len(optimized); opi++ { + result = append(result, optimized[opi]) + } + return result } + +// linesMatch returns true if two asmLines represent the same instruction. +func linesMatch(a, b asmLine) bool { + return a.isCode == b.isCode && a.isLabel == b.isLabel && a.isComment == b.isComment && a.text == b.text +} + +func countCodeLines(lines []asmLine) int { + n := 0 + for _, l := range lines { + if l.isCode { + n++ + } + } + return n +} diff --git a/internal/optimizer/optimizer.go b/internal/optimizer/optimizer.go index ce96937..770fd68 100644 --- a/internal/optimizer/optimizer.go +++ b/internal/optimizer/optimizer.go @@ -9,10 +9,7 @@ func Optimize(lines []string, cfg *Config) []string { } parsed := parseLines(lines) - - if cfg.Debug { - parsed = debugWithOriginals(parsed, "before peephole") - } + original := parsed if cfg.EnableLoad { parsed = passLoadElimination(parsed) @@ -27,10 +24,10 @@ func Optimize(lines []string, cfg *Config) []string { parsed = passSelfAssignment(parsed) } - if cfg.Debug { - parsed = debugWithOriginals(parsed, "after peephole") - } - parsed = stripOptMarkers(parsed) + if cfg.Debug { + original = stripOptMarkers(original) + parsed = debugDiff(original, parsed) + } return linesToString(parsed) } diff --git a/internal/optimizer/optimizer_test.go b/internal/optimizer/optimizer_test.go index b377194..a0b35d8 100644 --- a/internal/optimizer/optimizer_test.go +++ b/internal/optimizer/optimizer_test.go @@ -207,3 +207,46 @@ func TestOptimizeIntegration(t *testing.T) { t.Errorf("expected 5 lines, got %d:\n%v", len(output), output) } } + +func TestOptimizeWithDebug(t *testing.T) { + input := lines( + "\tlda x", + "; @@OPT:LINEAR:LET", + "\tlda x", + "\tsta y", + ) + + cfg := &Config{EnableLoad: true, Debug: true} + output := Optimize(input, cfg) + + // Header + 2 kept lines + 1 removed annotation = 4 + if len(output) != 4 { + t.Errorf("expected 4 lines, got %d:\n%v", len(output), output) + } + if !containsPrefix(output, "; --- peephole:") { + t.Errorf("expected peephole summary line:\n%v", output) + } + if !containsSubstring(output, "[removed]") { + t.Errorf("expected [removed] annotation:\n%v", output) + } +} + +func containsPrefix(lines []string, prefix string) bool { + for _, l := range lines { + if len(l) >= len(prefix) && l[:len(prefix)] == prefix { + return true + } + } + return false +} + +func containsSubstring(lines []string, sub string) bool { + for _, l := range lines { + for i := 0; i <= len(l)-len(sub); i++ { + if l[i:i+len(sub)] == sub { + return true + } + } + } + return false +} diff --git a/internal/optimizer/pass_imm.go b/internal/optimizer/pass_imm.go index 21af871..1539184 100644 --- a/internal/optimizer/pass_imm.go +++ b/internal/optimizer/pass_imm.go @@ -1,5 +1,7 @@ package optimizer +import "strings" + // passImmElimination removes redundant immediate loads. // Pattern: lda #N; ... (no A modification) ...; lda #N → remove second // Applies to A, X, Y. @@ -9,9 +11,6 @@ func passImmElimination(lines []asmLine) []asmLine { for _, line := range lines { if !line.isCode { - if line.optMarker { - continue - } if line.isLabel { rs.reset() } @@ -37,31 +36,42 @@ func passImmElimination(lines []asmLine) []asmLine { } func isRedundantImm(line asmLine, rs *regState) bool { + if !isImmediate(line.operand) { + return false + } + val := parseValueAfterHash(line.operand) + if val < 0 { + return false // unparseable (label ref, label) + } switch line.opcode { case "lda": - if isImmediate(line.operand) { - val := parseHexOrDec(line.operand[1:]) - if rs.a.val == val { - return true - } - rs.loadImm("a", val) + if rs.a.val == val { + return true } + rs.loadImm("a", val) case "ldx": - if isImmediate(line.operand) { - val := parseHexOrDec(line.operand[1:]) - if rs.x.val == val { - return true - } - rs.loadImm("x", val) + if rs.x.val == val { + return true } + rs.loadImm("x", val) case "ldy": - if isImmediate(line.operand) { - val := parseHexOrDec(line.operand[1:]) - if rs.y.val == val { - return true - } - rs.loadImm("y", val) + if rs.y.val == val { + return true } + rs.loadImm("y", val) } return false } + +// parseValueAfterHash extracts the numeric value after '#'. Returns -1 for unparseable. +func parseValueAfterHash(operand string) int { + if len(operand) < 2 { + return -1 + } + s := operand[1:] + // ACME high/low byte operators are not evaluable at optimize time + if strings.HasPrefix(s, "<") || strings.HasPrefix(s, ">") { + return -1 + } + return parseHexOrDec(s) +} diff --git a/internal/optimizer/pass_jmp.go b/internal/optimizer/pass_jmp.go index 5a7fe58..a6697a9 100644 --- a/internal/optimizer/pass_jmp.go +++ b/internal/optimizer/pass_jmp.go @@ -16,10 +16,6 @@ func passJmpNext(lines []asmLine) []asmLine { line := lines[i] - if skipJmpMarker(line) { - continue - } - // Check for jmp L followed by label L: if line.isCode && line.opcode == "jmp" && i+1 < len(lines) { next := lines[i+1] diff --git a/internal/optimizer/pass_load.go b/internal/optimizer/pass_load.go index b4164d5..3baa70b 100644 --- a/internal/optimizer/pass_load.go +++ b/internal/optimizer/pass_load.go @@ -1,5 +1,7 @@ package optimizer +import "strings" + // passLoadElimination removes redundant memory-to-register loads. // Pattern: lda X; ... (X not modified) ...; lda X → remove second // Applies to A, X, Y registers identically. @@ -9,9 +11,6 @@ func passLoadElimination(lines []asmLine) []asmLine { for _, line := range lines { if !line.isCode { - if line.optMarker { - continue - } if line.isLabel { rs.reset() } @@ -27,20 +26,30 @@ func passLoadElimination(lines []asmLine) []asmLine { // Check for redundant memory load if line.opcode == "lda" && !isImmediate(line.operand) { - if rs.a.src == line.operand { + if isIndexedOperand(line.operand) { + // Indexed/indirect addressing: address depends on X/Y, can't track by operand text alone + updateRegState(line, rs) + } else if rs.a.src == line.operand { continue // redundant, remove + } else { + rs.a.src = line.operand } - rs.a.src = line.operand } else if line.opcode == "ldx" && !isImmediate(line.operand) { - if rs.x.src == line.operand { + if isIndexedOperand(line.operand) { + updateRegState(line, rs) + } else if rs.x.src == line.operand { continue + } else { + rs.x.src = line.operand } - rs.x.src = line.operand } else if line.opcode == "ldy" && !isImmediate(line.operand) { - if rs.y.src == line.operand { + if isIndexedOperand(line.operand) { + updateRegState(line, rs) + } else if rs.y.src == line.operand { continue + } else { + rs.y.src = line.operand } - rs.y.src = line.operand } else { updateRegState(line, rs) } @@ -51,6 +60,13 @@ func passLoadElimination(lines []asmLine) []asmLine { return result } +// isIndexedOperand returns true for addressing modes where the effective address +// depends on X or Y registers (e.g. (zp),y, $xxxx,x, zp,x). +// Such loads can't be tracked by operand text alone since the register may change. +func isIndexedOperand(operand string) bool { + return strings.Contains(operand, "(") || strings.Contains(operand, ",") +} + func isImmediate(operand string) bool { return len(operand) > 0 && operand[0] == '#' } diff --git a/internal/optimizer/pass_self.go b/internal/optimizer/pass_self.go index 0e09552..943c77f 100644 --- a/internal/optimizer/pass_self.go +++ b/internal/optimizer/pass_self.go @@ -16,10 +16,6 @@ func passSelfAssignment(lines []asmLine) []asmLine { line := lines[i] - if skipJmpMarker(line) { - continue - } - // Check for lda X; sta X if line.isCode && line.opcode == "lda" && !isImmediate(line.operand) && i+1 < len(lines) { next := lines[i+1] diff --git a/internal/optimizer/regstate.go b/internal/optimizer/regstate.go index 6687c7e..da2adfb 100644 --- a/internal/optimizer/regstate.go +++ b/internal/optimizer/regstate.go @@ -92,19 +92,31 @@ func updateRegState(line asmLine, rs *regState) { switch line.opcode { case "lda": if strings.HasPrefix(line.operand, "#") { - rs.loadImm("a", parseHexOrDec(line.operand[1:])) + if val := parseHexOrDec(line.operand[1:]); val >= 0 { + rs.loadImm("a", val) + } else { + rs.resetA() + } } else { rs.loadMem("a", line.operand) } case "ldx": if strings.HasPrefix(line.operand, "#") { - rs.loadImm("x", parseHexOrDec(line.operand[1:])) + if val := parseHexOrDec(line.operand[1:]); val >= 0 { + rs.loadImm("x", val) + } else { + rs.resetX() + } } else { rs.loadMem("x", line.operand) } case "ldy": if strings.HasPrefix(line.operand, "#") { - rs.loadImm("y", parseHexOrDec(line.operand[1:])) + if val := parseHexOrDec(line.operand[1:]); val >= 0 { + rs.loadImm("y", val) + } else { + rs.resetY() + } } else { rs.loadMem("y", line.operand) } diff --git a/internal/optimizer/scanner.go b/internal/optimizer/scanner.go index 6af88bd..6dbe879 100644 --- a/internal/optimizer/scanner.go +++ b/internal/optimizer/scanner.go @@ -5,20 +5,32 @@ import ( "strings" ) -// parseHexOrDec parses an ACME-style number: $ff or 255 +// parseHexOrDec parses an ACME-style number: $ff or 255. +// Returns -1 if s is not a parseable simple number (e.g. label references, label). func parseHexOrDec(s string) int { if strings.HasPrefix(s, "$") { - v, _ := strconv.ParseInt(s[1:], 16, 16) + v, err := strconv.ParseInt(s[1:], 16, 16) + if err != nil { + return -1 + } return int(v) } // Strip trailing characters after the number (e.g., ",y" in "5,y") if idx := strings.IndexAny(s, ",)"); idx >= 0 { s = s[:idx] } - v, _ := strconv.ParseInt(s, 10, 16) + v, err := strconv.ParseInt(s, 10, 16) + if err != nil { + return -1 + } return int(v) } +// isSimpleNumber returns true if s is a plain hex/dec number (no label refs, <, >, etc.) +func isSimpleNumber(s string) bool { + return parseHexOrDec(s) >= 0 +} + type asmLine struct { text string isLabel bool