From d56eb397f935b0fa3ac31b3ea5c498613e26ea9d Mon Sep 17 00:00:00 2001
From: Irmen de Jong <irmen@razorvine.net>
Date: Mon, 6 Jun 2022 16:03:45 +0200
Subject: [PATCH] fix codegen for rol/ror on pointer indexed

---
 .../codegen/cpu6502/BuiltinFunctionsAsmGen.kt |  8 +++++-
 .../astprocessing/AstOnetimeTransforms.kt     | 26 +++++--------------
 docs/source/todo.rst                          |  4 +--
 3 files changed, 16 insertions(+), 22 deletions(-)

diff --git a/codeGenCpu6502/src/prog8/codegen/cpu6502/BuiltinFunctionsAsmGen.kt b/codeGenCpu6502/src/prog8/codegen/cpu6502/BuiltinFunctionsAsmGen.kt
index f804cefc2..ca05cedee 100644
--- a/codeGenCpu6502/src/prog8/codegen/cpu6502/BuiltinFunctionsAsmGen.kt
+++ b/codeGenCpu6502/src/prog8/codegen/cpu6502/BuiltinFunctionsAsmGen.kt
@@ -657,7 +657,13 @@ internal class BuiltinFunctionsAsmGen(private val program: Program,
     }
 
     private fun translateRolRorArrayArgs(arrayvar: IdentifierReference, indexer: ArrayIndex, operation: String, dt: Char) {
-        asmgen.assignExpressionToVariable(AddressOf(arrayvar, arrayvar.position), "prog8_lib.${operation}_array_u${dt}._arg_target", DataType.UWORD, null)
+        if(arrayvar.targetVarDecl(program)!!.datatype==DataType.UWORD) {
+            if(dt!='b')
+                throw AssemblyError("non-array var indexing requires bytes dt")
+            asmgen.assignExpressionToVariable(arrayvar, "prog8_lib.${operation}_array_u${dt}._arg_target", DataType.UWORD, null)
+        } else {
+            asmgen.assignExpressionToVariable(AddressOf(arrayvar, arrayvar.position), "prog8_lib.${operation}_array_u${dt}._arg_target", DataType.UWORD, null)
+        }
         asmgen.assignExpressionToVariable(indexer.indexExpr, "prog8_lib.${operation}_array_u${dt}._arg_index", DataType.UBYTE, null)
     }
 
diff --git a/compiler/src/prog8/compiler/astprocessing/AstOnetimeTransforms.kt b/compiler/src/prog8/compiler/astprocessing/AstOnetimeTransforms.kt
index b68b7df18..f57f839a4 100644
--- a/compiler/src/prog8/compiler/astprocessing/AstOnetimeTransforms.kt
+++ b/compiler/src/prog8/compiler/astprocessing/AstOnetimeTransforms.kt
@@ -35,29 +35,17 @@ internal class AstOnetimeTransforms(private val program: Program, private val op
     private fun replacePointerVarIndexWithMemreadOrMemwrite(arrayIndexedExpression: ArrayIndexedExpression, parent: Node): Iterable<IAstModification> {
         val arrayVar = arrayIndexedExpression.arrayvar.targetVarDecl(program)
         if(arrayVar!=null && arrayVar.datatype == DataType.UWORD) {
-            // rewrite   pointervar[index]  into  @(pointervar+index)
-            val indexer = arrayIndexedExpression.indexer
-            val add: Expression =
-            if(indexer.indexExpr.constValue(program)?.number==0.0)
-                arrayIndexedExpression.arrayvar.copy()
-            else
-                BinaryExpression(arrayIndexedExpression.arrayvar.copy(), "+", indexer.indexExpr, arrayIndexedExpression.position)
             if(parent is AssignTarget) {
-                // we're part of the target of an assignment, we have to actually change the assign target itself
+                // rewrite assignment target   pointervar[index]  into  @(pointervar+index)
+                val indexer = arrayIndexedExpression.indexer
+                val add: Expression =
+                if(indexer.indexExpr.constValue(program)?.number==0.0)
+                    arrayIndexedExpression.arrayvar.copy()
+                else
+                    BinaryExpression(arrayIndexedExpression.arrayvar.copy(), "+", indexer.indexExpr, arrayIndexedExpression.position)
                 val memwrite = DirectMemoryWrite(add, arrayIndexedExpression.position)
                 val newtarget = AssignTarget(null, null, memwrite, arrayIndexedExpression.position)
                 return listOf(IAstModification.ReplaceNode(parent, newtarget, parent.parent))
-            } else {
-                val fcall = parent as? IFunctionCall
-                if(fcall!=null) {
-                    // TODO currently, 6502 codegen is wrong when using pointer indexed args to an in-place modifying function.
-                    if(options.compTarget.name!=VMTarget.NAME
-                        && fcall.target.nameInSource.size==1
-                        && fcall.target.nameInSource[0] in InplaceModifyingBuiltinFunctions) {
-                        val memread = DirectMemoryRead(add, arrayIndexedExpression.position)
-                        return listOf(IAstModification.ReplaceNode(arrayIndexedExpression, memread, parent))
-                    }
-                }
             }
         }
 
diff --git a/docs/source/todo.rst b/docs/source/todo.rst
index 7293704b1..adcf266d4 100644
--- a/docs/source/todo.rst
+++ b/docs/source/todo.rst
@@ -3,8 +3,8 @@ TODO
 
 For next release
 ^^^^^^^^^^^^^^^^
-- optimize pointervar indexing codegen: fix 6502 codegen for params to in-place modifying functions (rol, ror, ...)
-- optimize pointervar indexing codegen: writing (all sorts of things)
+- vm: fix optimized pointervar indexing codegen when it is an assignment target (i.e. writing to ptr[ix])
+- 6502: fix optimized pointervar indexing codegen when it is an assignment target (i.e. writing to ptr[ix])
 - pipe operator: (targets other than 'Virtual'): allow non-unary function calls in the pipe that specify the other argument(s) in the calls.  Already working for VM target.
 - add McCarthy evaluation to shortcircuit and/or expressions. First do ifs by splitting them up? Then do expressions that compute a value?
 - Inliner: also inline function call expressions, and remove it from the StatementOptimizer