From 0e9a7e94ca3d3ac3779fa28d9e952e12eca6cd23 Mon Sep 17 00:00:00 2001 From: jespergravgaard Date: Fri, 6 Nov 2020 08:16:28 +0100 Subject: [PATCH] Fixed multiplexer problem caused by missing volatile on screen variable. Added test demonstrating the problem. --- src/main/kc/include/multiplexer.h | 2 +- src/main/kc/lib/multiplexer.c | 2 +- .../dk/camelot64/kickc/test/TestPrograms.java | 5 + src/test/kc/const-volatile-problem.c | 24 ++ src/test/ref/const-volatile-problem.asm | 42 +++ src/test/ref/const-volatile-problem.cfg | 34 ++ src/test/ref/const-volatile-problem.log | 356 ++++++++++++++++++ src/test/ref/const-volatile-problem.sym | 11 + 8 files changed, 474 insertions(+), 2 deletions(-) create mode 100644 src/test/kc/const-volatile-problem.c create mode 100644 src/test/ref/const-volatile-problem.asm create mode 100644 src/test/ref/const-volatile-problem.cfg create mode 100644 src/test/ref/const-volatile-problem.log create mode 100644 src/test/ref/const-volatile-problem.sym diff --git a/src/main/kc/include/multiplexer.h b/src/main/kc/include/multiplexer.h index 75b3d00d7..566ebb5cd 100644 --- a/src/main/kc/include/multiplexer.h +++ b/src/main/kc/include/multiplexer.h @@ -27,7 +27,7 @@ extern char PLEX_YPOS[PLEX_COUNT]; extern char PLEX_PTR[PLEX_COUNT]; // The address of the sprite pointers on the current screen (screen+0x3f8). -extern char* PLEX_SCREEN_PTR; +extern char* volatile PLEX_SCREEN_PTR; // Indexes of the plex-sprites sorted by sprite y-position. Each call to plexSort() will fix the sorting if changes to the Y-positions have ruined it. extern char PLEX_SORTED_IDX[PLEX_COUNT]; diff --git a/src/main/kc/lib/multiplexer.c b/src/main/kc/lib/multiplexer.c index f19f7d641..66b41cabf 100644 --- a/src/main/kc/lib/multiplexer.c +++ b/src/main/kc/lib/multiplexer.c @@ -28,7 +28,7 @@ char PLEX_YPOS[PLEX_COUNT]; char PLEX_PTR[PLEX_COUNT]; // The address of the sprite pointers on the current screen (screen+0x3f8). -char* PLEX_SCREEN_PTR = 0x400+0x3f8; +char* volatile PLEX_SCREEN_PTR = 0x400+0x3f8; // Indexes of the plex-sprites sorted by sprite y-position. Each call to plexSort() will fix the sorting if changes to the Y-positions have ruined it. char PLEX_SORTED_IDX[PLEX_COUNT]; diff --git a/src/test/java/dk/camelot64/kickc/test/TestPrograms.java b/src/test/java/dk/camelot64/kickc/test/TestPrograms.java index 0469c38e5..e54ddeb84 100644 --- a/src/test/java/dk/camelot64/kickc/test/TestPrograms.java +++ b/src/test/java/dk/camelot64/kickc/test/TestPrograms.java @@ -51,6 +51,11 @@ public class TestPrograms { // compileAndCompare("unknown-var-problem.c", log().verboseParse()); //} + @Test + public void testConstVolatileProblem1() throws IOException, URISyntaxException { + compileAndCompare("const-volatile-problem.c", log()); + } + @Test public void testFunctionPointerProblem1() throws IOException, URISyntaxException { compileAndCompare("function-pointer-problem-1.c"); diff --git a/src/test/kc/const-volatile-problem.c b/src/test/kc/const-volatile-problem.c new file mode 100644 index 000000000..4cdb162fc --- /dev/null +++ b/src/test/kc/const-volatile-problem.c @@ -0,0 +1,24 @@ +// Demonstrates problem where a pointer with constant value is never assigned - because it is only used in an IRQ +// PLEX_SCREEN_PTR1 is never assigned - while PLEX_SCREEN_PTR2 receives it's value. +// PLEX_SCREEN_PTR2 is saved by only being assigned once - thus is is identified as a constant. +// All assignments for PLEX_SCREEN_PTR1 are optimized away because it is only used in the IRQ. +// A potential fix is https://gitlab.com/camelot/kickc/-/issues/430 + +// The address of the sprite pointers on the current screen (screen+0x3f8). +char* PLEX_SCREEN_PTR1 = 0x400+0x3f8; +char* PLEX_SCREEN_PTR2 = 0x400+0x3f8; +volatile char plex_sprite_idx = 0; + +void()** const IRQ = 0x314; + +void main() { + PLEX_SCREEN_PTR1 = 0x400+0x3f8; + *IRQ = &irq; +} + +// Interrupt Routine +interrupt(kernel_keyboard) void irq() { + PLEX_SCREEN_PTR1[plex_sprite_idx] = 7; + PLEX_SCREEN_PTR2[plex_sprite_idx] = 7; + plex_sprite_idx++; +} diff --git a/src/test/ref/const-volatile-problem.asm b/src/test/ref/const-volatile-problem.asm new file mode 100644 index 000000000..bf619d9e9 --- /dev/null +++ b/src/test/ref/const-volatile-problem.asm @@ -0,0 +1,42 @@ +// Demonstrates problem where a pointer with constant value is never assigned - because it is only used in an IRQ +// PLEX_SCREEN_PTR1 is never assigned - while PLEX_SCREEN_PTR2 receives it's value. +// PLEX_SCREEN_PTR2 is saved by only being assigned once - thus is is identified as a constant. +// All assignments for PLEX_SCREEN_PTR1 are optimized away because it is only used in the IRQ. +// A potential fix is https://gitlab.com/camelot/kickc/-/issues/430 +.pc = $801 "Basic" +:BasicUpstart(__start) +.pc = $80d "Program" + .label IRQ = $314 + .label PLEX_SCREEN_PTR2 = $400+$3f8 + .label plex_sprite_idx = 4 + // The address of the sprite pointers on the current screen (screen+0x3f8). + .label PLEX_SCREEN_PTR1 = 2 +__start: { + // plex_sprite_idx = 0 + lda #0 + sta.z plex_sprite_idx + jsr main + rts +} +// Interrupt Routine +irq: { + // PLEX_SCREEN_PTR1[plex_sprite_idx] = 7 + lda #7 + ldy.z plex_sprite_idx + sta (PLEX_SCREEN_PTR1),y + // PLEX_SCREEN_PTR2[plex_sprite_idx] = 7 + sta PLEX_SCREEN_PTR2,y + // plex_sprite_idx++; + inc.z plex_sprite_idx + // } + jmp $ea31 +} +main: { + // *IRQ = &irq + lda #irq + sta IRQ+1 + // } + rts +} diff --git a/src/test/ref/const-volatile-problem.cfg b/src/test/ref/const-volatile-problem.cfg new file mode 100644 index 000000000..2f1d7b469 --- /dev/null +++ b/src/test/ref/const-volatile-problem.cfg @@ -0,0 +1,34 @@ + +void __start() +__start: scope:[__start] from + [0] phi() + to:__start::__init1 +__start::__init1: scope:[__start] from __start + [1] plex_sprite_idx = 0 + to:__start::@1 +__start::@1: scope:[__start] from __start::__init1 + [2] phi() + [3] call main + to:__start::@return +__start::@return: scope:[__start] from __start::@1 + [4] return + to:@return + +interrupt(KERNEL_KEYBOARD) void irq() +irq: scope:[irq] from + [5] PLEX_SCREEN_PTR1#6 = phi( ) + [6] PLEX_SCREEN_PTR1#6[plex_sprite_idx] = 7 + [7] PLEX_SCREEN_PTR2[plex_sprite_idx] = 7 + [8] plex_sprite_idx = ++ plex_sprite_idx + to:irq::@return +irq::@return: scope:[irq] from irq + [9] return + to:@return + +void main() +main: scope:[main] from __start::@1 + [10] *IRQ = &irq + to:main::@return +main::@return: scope:[main] from main + [11] return + to:@return diff --git a/src/test/ref/const-volatile-problem.log b/src/test/ref/const-volatile-problem.log new file mode 100644 index 000000000..2a4b50c20 --- /dev/null +++ b/src/test/ref/const-volatile-problem.log @@ -0,0 +1,356 @@ +Resolved forward reference irq to interrupt(KERNEL_KEYBOARD) void irq() +Inlined call call __init + +CONTROL FLOW GRAPH SSA + +void main() +main: scope:[main] from __start::@1 + PLEX_SCREEN_PTR1#0 = ((byte*)) $400+$3f8 + *IRQ = &irq + to:main::@return +main::@return: scope:[main] from main + PLEX_SCREEN_PTR1#5 = phi( main/PLEX_SCREEN_PTR1#0 ) + PLEX_SCREEN_PTR1#1 = PLEX_SCREEN_PTR1#5 + return + to:@return + +interrupt(KERNEL_KEYBOARD) void irq() +irq: scope:[irq] from + PLEX_SCREEN_PTR1#6 = phi( ) + PLEX_SCREEN_PTR1#6[plex_sprite_idx] = 7 + PLEX_SCREEN_PTR2[plex_sprite_idx] = 7 + plex_sprite_idx = ++ plex_sprite_idx + to:irq::@return +irq::@return: scope:[irq] from irq + return + to:@return + +void __start() +__start: scope:[__start] from + to:__start::__init1 +__start::__init1: scope:[__start] from __start + PLEX_SCREEN_PTR1#2 = (byte*)$400+$3f8 + plex_sprite_idx = 0 + to:__start::@1 +__start::@1: scope:[__start] from __start::__init1 + PLEX_SCREEN_PTR1#9 = phi( __start::__init1/PLEX_SCREEN_PTR1#2 ) + call main + to:__start::@2 +__start::@2: scope:[__start] from __start::@1 + PLEX_SCREEN_PTR1#7 = phi( __start::@1/PLEX_SCREEN_PTR1#1 ) + PLEX_SCREEN_PTR1#3 = PLEX_SCREEN_PTR1#7 + to:__start::@return +__start::@return: scope:[__start] from __start::@2 + PLEX_SCREEN_PTR1#8 = phi( __start::@2/PLEX_SCREEN_PTR1#3 ) + PLEX_SCREEN_PTR1#4 = PLEX_SCREEN_PTR1#8 + return + to:@return + +SYMBOL TABLE SSA +const nomodify void()** IRQ = (void()**)$314 +byte* PLEX_SCREEN_PTR1 +byte* PLEX_SCREEN_PTR1#0 +byte* PLEX_SCREEN_PTR1#1 +byte* PLEX_SCREEN_PTR1#2 +byte* PLEX_SCREEN_PTR1#3 +byte* PLEX_SCREEN_PTR1#4 +byte* PLEX_SCREEN_PTR1#5 +byte* PLEX_SCREEN_PTR1#6 +byte* PLEX_SCREEN_PTR1#7 +byte* PLEX_SCREEN_PTR1#8 +byte* PLEX_SCREEN_PTR1#9 +const byte* PLEX_SCREEN_PTR2 = (byte*)$400+$3f8 +void __start() +interrupt(KERNEL_KEYBOARD) void irq() +void main() +volatile byte plex_sprite_idx loadstore + +Adding number conversion cast (unumber) 7 in PLEX_SCREEN_PTR1#6[plex_sprite_idx] = 7 +Adding number conversion cast (unumber) 7 in PLEX_SCREEN_PTR2[plex_sprite_idx] = 7 +Successful SSA optimization PassNAddNumberTypeConversions +Inlining cast PLEX_SCREEN_PTR1#0 = (byte*)$400+$3f8 +Inlining cast PLEX_SCREEN_PTR1#6[plex_sprite_idx] = (unumber)7 +Inlining cast PLEX_SCREEN_PTR2[plex_sprite_idx] = (unumber)7 +Successful SSA optimization Pass2InlineCast +Simplifying constant pointer cast (void()**) 788 +Simplifying constant integer cast 7 +Simplifying constant integer cast 7 +Successful SSA optimization PassNCastSimplification +Finalized unsigned number type 7 +Finalized unsigned number type 7 +Successful SSA optimization PassNFinalizeNumberTypeConversions +Alias PLEX_SCREEN_PTR1#0 = PLEX_SCREEN_PTR1#5 PLEX_SCREEN_PTR1#1 +Alias PLEX_SCREEN_PTR1#2 = PLEX_SCREEN_PTR1#9 +Alias PLEX_SCREEN_PTR1#3 = PLEX_SCREEN_PTR1#7 PLEX_SCREEN_PTR1#8 PLEX_SCREEN_PTR1#4 +Successful SSA optimization Pass2AliasElimination +Identical Phi Values PLEX_SCREEN_PTR1#3 PLEX_SCREEN_PTR1#0 +Successful SSA optimization Pass2IdenticalPhiElimination +Constant right-side identified [0] PLEX_SCREEN_PTR1#0 = (byte*)$400+$3f8 +Successful SSA optimization Pass2ConstantRValueConsolidation +Constant PLEX_SCREEN_PTR1#0 = (byte*)$400+$3f8 +Constant PLEX_SCREEN_PTR1#2 = (byte*)$400+$3f8 +Successful SSA optimization Pass2ConstantIdentification +Eliminating unused constant PLEX_SCREEN_PTR1#0 +Eliminating unused constant PLEX_SCREEN_PTR1#2 +Successful SSA optimization PassNEliminateUnusedVars +Adding NOP phi() at start of __start +Adding NOP phi() at start of __start::@1 +Adding NOP phi() at start of __start::@2 +CALL GRAPH +Calls in [__start] to main:3 + +Created 1 initial phi equivalence classes +Coalesced down to 1 phi equivalence classes +Culled Empty Block label __start::@2 +Adding NOP phi() at start of __start +Adding NOP phi() at start of __start::@1 + +FINAL CONTROL FLOW GRAPH + +void __start() +__start: scope:[__start] from + [0] phi() + to:__start::__init1 +__start::__init1: scope:[__start] from __start + [1] plex_sprite_idx = 0 + to:__start::@1 +__start::@1: scope:[__start] from __start::__init1 + [2] phi() + [3] call main + to:__start::@return +__start::@return: scope:[__start] from __start::@1 + [4] return + to:@return + +interrupt(KERNEL_KEYBOARD) void irq() +irq: scope:[irq] from + [5] PLEX_SCREEN_PTR1#6 = phi( ) + [6] PLEX_SCREEN_PTR1#6[plex_sprite_idx] = 7 + [7] PLEX_SCREEN_PTR2[plex_sprite_idx] = 7 + [8] plex_sprite_idx = ++ plex_sprite_idx + to:irq::@return +irq::@return: scope:[irq] from irq + [9] return + to:@return + +void main() +main: scope:[main] from __start::@1 + [10] *IRQ = &irq + to:main::@return +main::@return: scope:[main] from main + [11] return + to:@return + + +VARIABLE REGISTER WEIGHTS +byte* PLEX_SCREEN_PTR1 +byte* PLEX_SCREEN_PTR1#6 2.0 +void __start() +interrupt(KERNEL_KEYBOARD) void irq() +void main() +volatile byte plex_sprite_idx loadstore 3.333333333333333 + +Initial phi equivalence classes +[ PLEX_SCREEN_PTR1#6 ] +Added variable plex_sprite_idx to live range equivalence class [ plex_sprite_idx ] +Complete equivalence classes +[ PLEX_SCREEN_PTR1#6 ] +[ plex_sprite_idx ] +Allocated zp[2]:2 [ PLEX_SCREEN_PTR1#6 ] +Allocated zp[1]:4 [ plex_sprite_idx ] +REGISTER UPLIFT POTENTIAL REGISTERS +Statement [1] plex_sprite_idx = 0 [ ] ( [ ] { } ) always clobbers reg byte a +Statement [6] PLEX_SCREEN_PTR1#6[plex_sprite_idx] = 7 [ plex_sprite_idx ] ( [ plex_sprite_idx ] { } ) always clobbers reg byte a reg byte y +Statement [7] PLEX_SCREEN_PTR2[plex_sprite_idx] = 7 [ plex_sprite_idx ] ( [ plex_sprite_idx ] { } ) always clobbers reg byte a reg byte y +Statement [10] *IRQ = &irq [ ] ( main:3 [ ] { } ) always clobbers reg byte a +Potential registers zp[2]:2 [ PLEX_SCREEN_PTR1#6 ] : zp[2]:2 , +Potential registers zp[1]:4 [ plex_sprite_idx ] : zp[1]:4 , + +REGISTER UPLIFT SCOPES +Uplift Scope [] 3.33: zp[1]:4 [ plex_sprite_idx ] 2: zp[2]:2 [ PLEX_SCREEN_PTR1#6 ] +Uplift Scope [main] +Uplift Scope [irq] +Uplift Scope [__start] + +Uplifting [] best 106 combination zp[1]:4 [ plex_sprite_idx ] zp[2]:2 [ PLEX_SCREEN_PTR1#6 ] +Uplifting [main] best 106 combination +Uplifting [irq] best 106 combination +Uplifting [__start] best 106 combination +Attempting to uplift remaining variables inzp[1]:4 [ plex_sprite_idx ] +Uplifting [] best 106 combination zp[1]:4 [ plex_sprite_idx ] + +ASSEMBLER BEFORE OPTIMIZATION + // File Comments +// Demonstrates problem where a pointer with constant value is never assigned - because it is only used in an IRQ +// PLEX_SCREEN_PTR1 is never assigned - while PLEX_SCREEN_PTR2 receives it's value. +// PLEX_SCREEN_PTR2 is saved by only being assigned once - thus is is identified as a constant. +// All assignments for PLEX_SCREEN_PTR1 are optimized away because it is only used in the IRQ. +// A potential fix is https://gitlab.com/camelot/kickc/-/issues/430 + // Upstart +.pc = $801 "Basic" +:BasicUpstart(__start) +.pc = $80d "Program" + // Global Constants & labels + .label IRQ = $314 + .label PLEX_SCREEN_PTR2 = $400+$3f8 + .label plex_sprite_idx = 4 + // The address of the sprite pointers on the current screen (screen+0x3f8). + .label PLEX_SCREEN_PTR1 = 2 + // __start +__start: { + jmp __init1 + // __start::__init1 + __init1: + // [1] plex_sprite_idx = 0 -- vbuz1=vbuc1 + lda #0 + sta.z plex_sprite_idx + // [2] phi from __start::__init1 to __start::@1 [phi:__start::__init1->__start::@1] + __b1_from___init1: + jmp __b1 + // __start::@1 + __b1: + // [3] call main + jsr main + jmp __breturn + // __start::@return + __breturn: + // [4] return + rts +} + // irq +// Interrupt Routine +irq: { + // entry interrupt(KERNEL_KEYBOARD) + // [6] PLEX_SCREEN_PTR1#6[plex_sprite_idx] = 7 -- pbuz1_derefidx_vbuz2=vbuc1 + lda #7 + ldy.z plex_sprite_idx + sta (PLEX_SCREEN_PTR1),y + // [7] PLEX_SCREEN_PTR2[plex_sprite_idx] = 7 -- pbuc1_derefidx_vbuz1=vbuc2 + lda #7 + ldy.z plex_sprite_idx + sta PLEX_SCREEN_PTR2,y + // [8] plex_sprite_idx = ++ plex_sprite_idx -- vbuz1=_inc_vbuz1 + inc.z plex_sprite_idx + jmp __breturn + // irq::@return + __breturn: + // [9] return - exit interrupt(KERNEL_KEYBOARD) + jmp $ea31 +} + // main +main: { + // [10] *IRQ = &irq -- _deref_qprc1=pprc2 + lda #irq + sta IRQ+1 + jmp __breturn + // main::@return + __breturn: + // [11] return + rts +} + // File Data + +ASSEMBLER OPTIMIZATIONS +Removing instruction jmp __init1 +Removing instruction jmp __b1 +Removing instruction jmp __breturn +Removing instruction jmp __breturn +Removing instruction jmp __breturn +Succesful ASM optimization Pass5NextJumpElimination +Removing instruction lda #7 +Removing instruction ldy.z plex_sprite_idx +Succesful ASM optimization Pass5UnnecesaryLoadElimination +Removing instruction __b1_from___init1: +Succesful ASM optimization Pass5RedundantLabelElimination +Removing instruction __init1: +Removing instruction __b1: +Removing instruction __breturn: +Removing instruction __breturn: +Removing instruction __breturn: +Succesful ASM optimization Pass5UnusedLabelElimination + +FINAL SYMBOL TABLE +const nomodify void()** IRQ = (void()**) 788 +byte* PLEX_SCREEN_PTR1 +byte* PLEX_SCREEN_PTR1#6 PLEX_SCREEN_PTR1 zp[2]:2 2.0 +const byte* PLEX_SCREEN_PTR2 = (byte*)$400+$3f8 +void __start() +interrupt(KERNEL_KEYBOARD) void irq() +void main() +volatile byte plex_sprite_idx loadstore zp[1]:4 3.333333333333333 + +zp[2]:2 [ PLEX_SCREEN_PTR1#6 ] +zp[1]:4 [ plex_sprite_idx ] + + +FINAL ASSEMBLER +Score: 59 + + // File Comments +// Demonstrates problem where a pointer with constant value is never assigned - because it is only used in an IRQ +// PLEX_SCREEN_PTR1 is never assigned - while PLEX_SCREEN_PTR2 receives it's value. +// PLEX_SCREEN_PTR2 is saved by only being assigned once - thus is is identified as a constant. +// All assignments for PLEX_SCREEN_PTR1 are optimized away because it is only used in the IRQ. +// A potential fix is https://gitlab.com/camelot/kickc/-/issues/430 + // Upstart +.pc = $801 "Basic" +:BasicUpstart(__start) +.pc = $80d "Program" + // Global Constants & labels + .label IRQ = $314 + .label PLEX_SCREEN_PTR2 = $400+$3f8 + .label plex_sprite_idx = 4 + // The address of the sprite pointers on the current screen (screen+0x3f8). + .label PLEX_SCREEN_PTR1 = 2 + // __start +__start: { + // __start::__init1 + // plex_sprite_idx = 0 + // [1] plex_sprite_idx = 0 -- vbuz1=vbuc1 + lda #0 + sta.z plex_sprite_idx + // [2] phi from __start::__init1 to __start::@1 [phi:__start::__init1->__start::@1] + // __start::@1 + // [3] call main + jsr main + // __start::@return + // [4] return + rts +} + // irq +// Interrupt Routine +irq: { + // entry interrupt(KERNEL_KEYBOARD) + // PLEX_SCREEN_PTR1[plex_sprite_idx] = 7 + // [6] PLEX_SCREEN_PTR1#6[plex_sprite_idx] = 7 -- pbuz1_derefidx_vbuz2=vbuc1 + lda #7 + ldy.z plex_sprite_idx + sta (PLEX_SCREEN_PTR1),y + // PLEX_SCREEN_PTR2[plex_sprite_idx] = 7 + // [7] PLEX_SCREEN_PTR2[plex_sprite_idx] = 7 -- pbuc1_derefidx_vbuz1=vbuc2 + sta PLEX_SCREEN_PTR2,y + // plex_sprite_idx++; + // [8] plex_sprite_idx = ++ plex_sprite_idx -- vbuz1=_inc_vbuz1 + inc.z plex_sprite_idx + // irq::@return + // } + // [9] return - exit interrupt(KERNEL_KEYBOARD) + jmp $ea31 +} + // main +main: { + // *IRQ = &irq + // [10] *IRQ = &irq -- _deref_qprc1=pprc2 + lda #irq + sta IRQ+1 + // main::@return + // } + // [11] return + rts +} + // File Data + diff --git a/src/test/ref/const-volatile-problem.sym b/src/test/ref/const-volatile-problem.sym new file mode 100644 index 000000000..6b4279080 --- /dev/null +++ b/src/test/ref/const-volatile-problem.sym @@ -0,0 +1,11 @@ +const nomodify void()** IRQ = (void()**) 788 +byte* PLEX_SCREEN_PTR1 +byte* PLEX_SCREEN_PTR1#6 PLEX_SCREEN_PTR1 zp[2]:2 2.0 +const byte* PLEX_SCREEN_PTR2 = (byte*)$400+$3f8 +void __start() +interrupt(KERNEL_KEYBOARD) void irq() +void main() +volatile byte plex_sprite_idx loadstore zp[1]:4 3.333333333333333 + +zp[2]:2 [ PLEX_SCREEN_PTR1#6 ] +zp[1]:4 [ plex_sprite_idx ]