get rid of problematic common-subexpression optimization

codeCore/src/prog8/code/optimize/Optimizer.kt

@@ -10,9 +10,8 @@ fun optimizeIntermediateAst(program: PtProgram, options: CompilationOptions, st:
     if (!options.optimize)
         return
     while (errors.noErrors() &&
-        (optimizeCommonSubExpressions(program, errors)
-                + optimizeBitTest(program, options)
-                + optimizeAssignTargets(program, st, errors)) > 0
+            (optimizeBitTest(program, options)
+            + optimizeAssignTargets(program, st, errors)) > 0
     ) {
         // keep rolling
     }
@@ -28,97 +27,6 @@ private fun walkAst(root: PtNode, act: (node: PtNode, depth: Int) -> Boolean) {
 }
 
 
-private var tempVarCounter = 0
-
-private fun optimizeCommonSubExpressions(program: PtProgram, errors: IErrorReporter): Int {
-
-    fun extractableSubExpr(expr: PtExpression): Boolean {
-        if(expr is PtArrayIndexer && expr.index.isSimple())
-            return false
-        if (expr is PtMemoryByte && expr.address.isSimple())
-            return false
-
-        val result = if(expr is PtBinaryExpression)
-            expr.type !in ByteDatatypes ||
-            !(expr.left.isSimple() && expr.right.isSimple()) ||
-            (expr.operator !in LogicalOperators && expr.operator !in BitwiseOperators)
-        else if (expr is PtArrayIndexer && expr.type !in ByteDatatypes)
-            true
-        else
-            !expr.isSimple()
-        return result
-    }
-
-    // for each Binaryexpression, recurse to find a common subexpression pair therein.
-    val commons = mutableMapOf<PtBinaryExpression, Pair<PtExpression, PtExpression>>()
-    walkAst(program) { node: PtNode, depth: Int ->
-        if(node is PtBinaryExpression) {
-            val subExpressions = mutableListOf<PtExpression>()
-            walkAst(node.left) { subNode: PtNode, subDepth: Int ->
-                if (subNode is PtExpression) {
-                    if(extractableSubExpr(subNode)) subExpressions.add(subNode)
-                    true
-                } else false
-            }
-            walkAst(node.right) { subNode: PtNode, subDepth: Int ->
-                if (subNode is PtExpression) {
-                    if(extractableSubExpr(subNode)) subExpressions.add(subNode)
-                    true
-                } else false
-            }
-
-            outer@for (first in subExpressions) {
-                for (second in subExpressions) {
-                    if (first!==second && first isSameAs second) {
-                        commons[node] = first to second
-                        break@outer     // do only 1 replacement at a time per binaryexpression
-                    }
-                }
-            }
-            false
-        } else true
-    }
-
-    // replace common subexpressions by a temp variable that is assigned only once.
-    // TODO: check for commonalities across multiple separate expressions in the current scope, not only inside a single line
-    commons.forEach { binexpr, (occurrence1, occurrence2) ->
-        val (stmtContainer, stmt) = findContainingStatements(binexpr)
-        val occurrence1idx = occurrence1.parent.children.indexOf(occurrence1)
-        val occurrence2idx = occurrence2.parent.children.indexOf(occurrence2)
-        val containerScopedName = findScopeName(stmtContainer)
-        tempVarCounter++
-        val tempvarName = "prog8_subexprvar_$tempVarCounter"
-        // TODO: some tempvars could be reused, if they are from different lines
-
-        val datatype = occurrence1.type
-        val singleReplacement1 = PtIdentifier("$containerScopedName.$tempvarName", datatype, occurrence1.position)
-        val singleReplacement2 = PtIdentifier("$containerScopedName.$tempvarName", datatype, occurrence2.position)
-        occurrence1.parent.children[occurrence1idx] = singleReplacement1
-        singleReplacement1.parent = occurrence1.parent
-        occurrence2.parent.children[occurrence2idx] = singleReplacement2
-        singleReplacement2.parent = occurrence2.parent
-
-        val tempassign = PtAssignment(binexpr.position).also { assign ->
-            assign.add(PtAssignTarget(false, binexpr.position).also { tgt->
-                tgt.add(PtIdentifier("$containerScopedName.$tempvarName", datatype, binexpr.position))
-            })
-            assign.add(occurrence1)
-            occurrence1.parent = assign
-        }
-        stmtContainer.children.add(stmtContainer.children.indexOf(stmt), tempassign)
-        tempassign.parent = stmtContainer
-
-        val tempvar = PtVariable(tempvarName, datatype, ZeropageWish.NOT_IN_ZEROPAGE, null, null, binexpr.position)
-        stmtContainer.add(0, tempvar)
-        tempvar.parent = stmtContainer
-
-        // errors.info("common subexpressions replaced by a tempvar, maybe simplify the expression manually", binexpr.position)
-    }
-
-    return commons.size
-}
-
-
 private fun optimizeAssignTargets(program: PtProgram, st: SymbolTable, errors: IErrorReporter): Int {
     var changes = 0
     walkAst(program) { node: PtNode, depth: Int ->
@@ -235,24 +143,3 @@ internal fun isSame(identifier: PtIdentifier, type: DataType, returnedRegister:
     }
     return false   // there are no identifiers directly corresponding to cpu registers
 }
-
-
-internal fun findScopeName(node: PtNode): String {
-    var parent=node
-    while(parent !is PtNamedNode)
-        parent = parent.parent
-    return parent.scopedName
-}
-
-
-internal fun findContainingStatements(node: PtNode): Pair<PtNode, PtNode> {      // returns (parentstatementcontainer, childstatement)
-    var parent = node.parent
-    var child = node
-    while(true) {
-        if(parent is IPtStatementContainer) {
-            return parent to child
-        }
-        child=parent
-        parent=parent.parent
-    }
-}

compiler/src/prog8/compiler/astprocessing/VariousCleanups.kt

@@ -149,8 +149,8 @@ internal class VariousCleanups(val program: Program, val errors: IErrorReporter,
                             return listOf(IAstModification.ReplaceNode(expr, compare, parent))
                         }
                     }
-                    if(values.size<4)
-                        return noModifications  // replacement only worthwhile for 4 or more values
+                    if(values.size<3)
+                        return noModifications  // replacement only worthwhile for 3 or more values
                     val valueCopies = values.sortedBy { it.number }.map { it.copy() }
                     val arrayType = ElementToArrayTypes.getValue(elementType)
                     val valuesArray = ArrayLiteral(InferredTypes.InferredType.known(arrayType), valueCopies.toTypedArray(), expr.position)

docs/source/todo.rst

@@ -1,12 +1,9 @@
 TODO
 ====
 
-Badapple has become 200 bytes LARGER than before!!
-
 Fix testgfx2 screen text being uppercase (should be upper+lowercased)
 
-assembler seems to crash on hello4...
-
+diskio.internal_f_tell gets included in the assembly even though f_tell is never called ??? (when using another routine from diskio...)
 
 IR: Improve codegen for for loops downto 0.  (BPL if <=127 etc like 6502 codegen?)
 
@@ -81,7 +78,6 @@ Optimizations:
   for instance, vars used inside loops first, then loopvars, then uwords used as pointers, then the rest
 - various optimizers skip stuff if compTarget.name==VMTarget.NAME.  Once 6502-codegen is done from IR code,
   those checks should probably be removed, or be made permanent
-- optimizeCommonSubExpressions: currently only looks in expressions on a single line, could search across multiple expressions
 
 STRUCTS?
 --------

examples/test.p8

@@ -1,51 +1,15 @@
-%import textio
 %zeropage basicsafe
 %option no_sysinit
 
 
 main {
     sub start() {
-        ubyte x
-        uword w
-        uword @shared wstart=1000
-        ubyte @shared bstart=100
-        uword y
-        uword duration
-        byte b
+        ubyte @shared x
 
+;        if x==13 or x==42               ; why is there shortcut-evaluation here for those simple terms
+;            cx16.r0L++
 
-        cbm.SETTIM(0,0,0)
-        repeat 1000 {
-            y=0
-            for w in wstart downto 0 {
-                y++
-            }
-            for w in wstart downto 1 {
-                y++
-            }
-            ; TODO words
-        }
-        txt.print_uw(cbm.RDTIM16())
-        if y!=2001
-            txt.print("error\n")
-
-        ; without new loops: $26e, 482 jiffies
-
-/*
-        for w in 65535 downto 0 {
-            y++
-        }
-        if y!=0
-            txt.print("error 10\n")
-
-        y=0
-        for w in 0 to 65535 {
-            y++
-        }
-        if y!=0
-            txt.print("error 11\n")
-*/
-
-        txt.print("\nall done\n")
+        if x==13 or x==42 or x==99 or x==100     ; is this really more efficient as a containment check , when the shortcut-evaluation gets fixed??
+            cx16.r0L++
     }
 }