From 01c67549288a85c86c406950ed133f3c82afea22 Mon Sep 17 00:00:00 2001 From: Irmen de Jong Date: Wed, 11 Sep 2024 01:08:18 +0200 Subject: [PATCH] get rid of problematic common-subexpression optimization --- codeCore/src/prog8/code/optimize/Optimizer.kt | 117 +----------------- .../compiler/astprocessing/VariousCleanups.kt | 4 +- docs/source/todo.rst | 6 +- examples/test.p8 | 46 +------ 4 files changed, 10 insertions(+), 163 deletions(-) diff --git a/codeCore/src/prog8/code/optimize/Optimizer.kt b/codeCore/src/prog8/code/optimize/Optimizer.kt index e6daebdf9..4b02e8d37 100644 --- a/codeCore/src/prog8/code/optimize/Optimizer.kt +++ b/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>() - walkAst(program) { node: PtNode, depth: Int -> - if(node is PtBinaryExpression) { - val subExpressions = mutableListOf() - 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 { // 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 - } -} diff --git a/compiler/src/prog8/compiler/astprocessing/VariousCleanups.kt b/compiler/src/prog8/compiler/astprocessing/VariousCleanups.kt index 55f8fe1d5..a3450ea56 100644 --- a/compiler/src/prog8/compiler/astprocessing/VariousCleanups.kt +++ b/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) diff --git a/docs/source/todo.rst b/docs/source/todo.rst index 30140d4be..a9750fdb5 100644 --- a/docs/source/todo.rst +++ b/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? -------- diff --git a/examples/test.p8 b/examples/test.p8 index 94271791c..4aef82977 100644 --- a/examples/test.p8 +++ b/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++ } }