From f0f6150e18fa6c48e1a8ad2cc83336b866dde810 Mon Sep 17 00:00:00 2001
From: Irmen de Jong <irmen@razorvine.net>
Date: Wed, 23 Dec 2020 02:30:46 +0100
Subject: [PATCH] fix problem with reuse of auto-indexer-variables that could
 result in wrong code for routines using multiple array indexings

---
 .../ast/processing/StatementReorderer.kt      | 28 ++++++++-----------
 .../src/prog8/ast/statements/AstStatements.kt |  2 +-
 docs/source/todo.rst                          |  1 -
 examples/cx16/gfx2.p8                         | 25 +++++++++++++----
 4 files changed, 31 insertions(+), 25 deletions(-)

diff --git a/compiler/src/prog8/ast/processing/StatementReorderer.kt b/compiler/src/prog8/ast/processing/StatementReorderer.kt
index 5d2742017..e78eb18ec 100644
--- a/compiler/src/prog8/ast/processing/StatementReorderer.kt
+++ b/compiler/src/prog8/ast/processing/StatementReorderer.kt
@@ -156,28 +156,22 @@ internal class StatementReorderer(val program: Program, val errors: ErrorReporte
         val indexerVarPrefix = "prog8_autovar_index_"
         val repo = subroutine.asmGenInfo.usedAutoArrayIndexerForStatements
 
-        // TODO make this even smarter so that an indexerVar can be reused for a different following statement...  requires updating the partOfStatement?
-        var indexerVar = repo.firstOrNull { it.replaces isSameAs expr.indexer }
-        if(indexerVar==null) {
-            // add another loop index var to be used for this expression
-            val indexerVarName = "$indexerVarPrefix${expr.indexer.hashCode()}"
-            indexerVar = AsmGenInfo.ArrayIndexerInfo(indexerVarName, expr.indexer, statement)
-            repo.add(indexerVar)
-            // create the indexer var at block level scope
-            val vardecl = VarDecl(VarDeclType.VAR, DataType.UBYTE, ZeropageWish.PREFER_ZEROPAGE,
-                    null, indexerVarName, null, null, isArray = false, autogeneratedDontRemove = true, position = expr.position)
-            modifications.add(IAstModification.InsertFirst(vardecl, subroutine))
-        }
-        indexerVar.used++       // keep track of how many times it it used, to avoid assigning it multiple times
+        // TODO make this a bit smarter so it can reuse indexer variables. BUT BEWARE of scoping+initialization problems then
+        // add another loop index var to be used for this expression
+        val indexerVarName = "$indexerVarPrefix${expr.indexer.hashCode()}"
+        val indexerVar = AsmGenInfo.ArrayIndexerInfo(indexerVarName, expr.indexer)
+        repo.add(indexerVar)
+        // create the indexer var at block level scope
+        val vardecl = VarDecl(VarDeclType.VAR, DataType.UBYTE, ZeropageWish.PREFER_ZEROPAGE,
+                null, indexerVarName, null, null, isArray = false, autogeneratedDontRemove = true, position = expr.position)
+        modifications.add(IAstModification.InsertFirst(vardecl, subroutine))
 
         // replace the indexer with just the variable
         // assign the indexing expression to the helper variable, but only if that hasn't been done already
         val indexerExpression = expr.indexer.origExpression!!
         val target = AssignTarget(IdentifierReference(listOf(indexerVar.name), indexerExpression.position), null, null, indexerExpression.position)
-        if(indexerVar.used==1) {
-            val assign = Assignment(target, indexerExpression, indexerExpression.position)
-            modifications.add(IAstModification.InsertBefore(statement, assign, statement.definingScope()))
-        }
+        val assign = Assignment(target, indexerExpression, indexerExpression.position)
+        modifications.add(IAstModification.InsertBefore(statement, assign, statement.definingScope()))
         modifications.add(IAstModification.SetExpression( {
             expr.indexer.indexVar = it as IdentifierReference
             expr.indexer.indexNum = null
diff --git a/compiler/src/prog8/ast/statements/AstStatements.kt b/compiler/src/prog8/ast/statements/AstStatements.kt
index 1afb020f0..96681b6b0 100644
--- a/compiler/src/prog8/ast/statements/AstStatements.kt
+++ b/compiler/src/prog8/ast/statements/AstStatements.kt
@@ -698,7 +698,7 @@ class AsmGenInfo {
     var usedFloatEvalResultVar1 = false
     var usedFloatEvalResultVar2 = false
 
-    class ArrayIndexerInfo(val name: String, val replaces: ArrayIndex, val partOfStatement: Statement, var used: Int=0)
+    class ArrayIndexerInfo(val name: String, val replaces: ArrayIndex)
 }
 
 // the subroutine class covers both the normal user-defined subroutines,
diff --git a/docs/source/todo.rst b/docs/source/todo.rst
index a859b1140..8560cd105 100644
--- a/docs/source/todo.rst
+++ b/docs/source/todo.rst
@@ -3,7 +3,6 @@ TODO
 ====
 
 - optimize (byte) bitshifting 1<<x (and 2**x) via lookup table 1,2,4,8,...
-- Cx16 target: support full-screen 640x480 and 320x240 graphics? That requires our own custom graphics routines though to draw lines, and plot pixels.
 - hoist all variable declarations up to the subroutine scope *before* even the constant folding takes place (to avoid undefined symbol errors when referring to a variable from another nested scope in the subroutine)
 - make it possible to use cpu opcodes such as 'nop' as variable names by prefixing all asm vars with something such as '_'
 - option to load the built-in library files from a directory instead of the embedded ones (for easier library development/debugging)
diff --git a/examples/cx16/gfx2.p8 b/examples/cx16/gfx2.p8
index 065a27ce0..d050d97ef 100644
--- a/examples/cx16/gfx2.p8
+++ b/examples/cx16/gfx2.p8
@@ -1,18 +1,29 @@
 %import textio
-%import diskio
-%import floats
 %import graphics
 %import test_stack
-%zeropage basicsafe
 %option no_sysinit
 
+; TODO full-screen graphics mode library, in development.  (as replacement for the graphics routines in ROM that are constrained to 200 vertical pixels and lores mode only)
+
+
 main {
 
     sub start () {
+        ubyte[] modes = [0, 1, 128]
+        ubyte mode
+        for mode in modes {
+            gfx2.set_mode(mode)
+            gfx2.clear_screen()
+            draw()
+            cx16.wait(120)
+        }
 
-        gfx2.set_mode(128)
-        gfx2.clear_screen()
+        repeat {
+            ;
+        }
+    }
 
+    sub draw() {
         uword offset
         ubyte angle
         uword x
@@ -42,11 +53,13 @@ main {
 
 gfx2 {
 
+    ; read-only control variables:
     ubyte active_mode = 255
     uword width = 0
     uword height = 0
     ubyte bpp = 0
 
+
     sub set_mode(ubyte mode) {
         ; mode 0 = bitmap 320 x 240 x 1c monochrome
         ; mode 1 = bitmap 320 x 240 x 256c
@@ -130,7 +143,7 @@ gfx2 {
         ubyte[8] bits = [128, 64, 32, 16, 8, 4, 2, 1]
         when active_mode {
             0 -> {
-                cx16.vpoke_or(0, y*(320/8) + x/8, bits[lsb(x)&7])           ; TODO !?!? if the &7 remains, the code at the '128' label is wrong!!! if changed or removed, the code at 128 works fine!
+                cx16.vpoke_or(0, y*(320/8) + x/8, bits[lsb(x)&7])
             }
             1 -> {
                 void addr_mul_320_add_24(y, x)      ; 24 bits result is in r0 and r1L