From f965804e6d898562509e54e4d8df088a0ce0f68b Mon Sep 17 00:00:00 2001
From: Irmen de Jong <irmen@razorvine.net>
Date: Fri, 28 Jan 2022 16:27:11 +0100
Subject: [PATCH] fix invalid optimization of returning a parameter variable in
 a subroutine

---
 .../src/prog8/optimizer/StatementOptimizer.kt | 11 ++++++----
 docs/source/todo.rst                          | 21 ++++---------------
 examples/test.p8                              |  9 +++++---
 3 files changed, 17 insertions(+), 24 deletions(-)

diff --git a/codeOptimizers/src/prog8/optimizer/StatementOptimizer.kt b/codeOptimizers/src/prog8/optimizer/StatementOptimizer.kt
index 64d830d7e..ed233db13 100644
--- a/codeOptimizers/src/prog8/optimizer/StatementOptimizer.kt
+++ b/codeOptimizers/src/prog8/optimizer/StatementOptimizer.kt
@@ -18,7 +18,7 @@ class StatementOptimizer(private val program: Program,
 ) : AstWalker() {
 
     override fun before(functionCallExpr: FunctionCallExpression, parent: Node): Iterable<IAstModification> {
-        // if the first instruction in the called subroutine is a return statement with a simple value,
+        // if the first instruction in the called subroutine is a return statement with a simple value (NOT being a parameter),
         // remove the jump altogeter and inline the returnvalue directly.
 
         fun scopePrefix(variable: IdentifierReference): IdentifierReference {
@@ -41,7 +41,12 @@ class StatementOptimizer(private val program: Program,
                             else -> return noModifications
                         }
                     }
-                    is IdentifierReference -> scopePrefix(orig)
+                    is IdentifierReference -> {
+                        if(orig.targetVarDecl(program)?.origin == VarDeclOrigin.SUBROUTINEPARAM)
+                            return noModifications
+                        else
+                            scopePrefix(orig)
+                    }
                     is NumericLiteralValue -> orig.copy()
                     is StringLiteralValue -> orig.copy()
                     else -> return noModifications
@@ -52,8 +57,6 @@ class StatementOptimizer(private val program: Program,
         return noModifications
     }
 
-
-
     override fun after(functionCallStatement: FunctionCallStatement, parent: Node): Iterable<IAstModification> {
         if(functionCallStatement.target.targetStatement(program) is BuiltinFunctionPlaceholder) {
             val functionName = functionCallStatement.target.nameInSource[0]
diff --git a/docs/source/todo.rst b/docs/source/todo.rst
index 0a61f9711..790dc7a4a 100644
--- a/docs/source/todo.rst
+++ b/docs/source/todo.rst
@@ -3,19 +3,8 @@ TODO
 
 For next release
 ^^^^^^^^^^^^^^^^
-fix the value of ww being wrong (with optimizations enabled) in :
-    sub start() {
-        byte ub1 = -50
-        byte ub2 = -51
-        byte ub3 = -52
-        byte ub4 = -53
-        word ww = func(ub1, ub2, ub3, ub4)
-        txt.print_w(ww)
-    }
-
-    sub func(word x1, word y1, word x2, word y2) -> word {
-        return x1
-    }
+- Fix: don't report as recursion if code assigns address of its own subroutine to something, rather than calling it
+- nameInAssemblyCode() should search smarter (only labels in column 0? only full words, not part of a larger word? use regex? )
 
 
 Need help with
@@ -33,12 +22,10 @@ Blocked by an official Commander-x16 r39 release
 
 Future Things and Ideas
 ^^^^^^^^^^^^^^^^^^^^^^^
-- nameInAssemblyCode() should search smarter (only labels in column 0? only full words, not part of a larger word?)
-- Fix: don't report as recursion if code assigns address of its own subroutine to something, rather than calling it
 - allow "xxx" * constexpr  (where constexpr is not a number literal, now gives expression error not same type)
-- can we promise a left-to-right function call argument evaluation? without sacrificing performance
+- can we promise a left-to-right function call argument evaluation? without sacrificing performance. note: C/C++ don't guarantee anything about this
 - unify FunctioncallExpression + FunctioncallStatement and PipeExpression + Pipe statement, may require moving Expression/Statement into interfaces instead of abstract base classes
-- for the pipe operator: recognise a placeholder (``?`` or ``%`` or ``_``) in a non-unary function call to allow things as ``4 |> mkword(?, $44) |> print_uw``
+- for the pipe operator: recognise a placeholder (``?`` or ``%`` or ``_``) in a non-unary function call to allow non-unary functions in the chain; ``4 |> mkword(?, $44) |> print_uw``
 - make it possible to use cpu opcodes such as 'nop' as variable names by prefixing all asm vars with something such as ``v_``
   then we can get rid of the instruction lists in the machinedefinitions as well?
 - make it possible to inline non-asmsub routines that just contain a single statement (return, functioncall, assignment)
diff --git a/examples/test.p8 b/examples/test.p8
index 6d5d99e82..1cabade78 100644
--- a/examples/test.p8
+++ b/examples/test.p8
@@ -8,20 +8,23 @@ main {
 
         word ww = func(bb1, bb2)
         txt.print_w(ww)
-        txt.print(" <- must be -50\n")          ; TODO fix this with noopt (prints 0) !
+        txt.print(" <- must be -50\n")
 
         ubyte ub1 = 50
         ubyte ub2 = 51
         uword uw = funcu(ub1, ub2)
         txt.print_uw(uw)
-        txt.print(" <- must be 50\n")            ; TODO fix this with noopt (prints 0) !
+        txt.print(" <- must be 50\n")
     }
 
     sub func(word x1, word y1) -> word {
         return x1
+        ;word zz = x1+1
+        ;return zz-1
     }
 
     sub funcu(uword x1, uword y1) -> uword {
-        return x1
+        uword zz = x1+1
+        return zz-1
     }
 }