From 24d13dd120992f87a47723129ed44c655820d0cc Mon Sep 17 00:00:00 2001 From: Irmen de Jong Date: Sat, 2 Jul 2022 00:54:38 +0200 Subject: [PATCH] fix problematic optimizations to logical expressions --- .../compiler/astprocessing/AstPreprocessor.kt | 11 ---- .../astprocessing/NotExpressionChanger.kt | 55 +++---------------- compiler/test/TestOptimization.kt | 22 ++------ docs/source/todo.rst | 4 -- examples/test.p8 | 52 +++++++++--------- 5 files changed, 37 insertions(+), 107 deletions(-) diff --git a/compiler/src/prog8/compiler/astprocessing/AstPreprocessor.kt b/compiler/src/prog8/compiler/astprocessing/AstPreprocessor.kt index 1194a3057..e7b38bbff 100644 --- a/compiler/src/prog8/compiler/astprocessing/AstPreprocessor.kt +++ b/compiler/src/prog8/compiler/astprocessing/AstPreprocessor.kt @@ -115,17 +115,6 @@ class AstPreprocessor(val program: Program, val errors: IErrorReporter, val comp return noModifications } - override fun after(expr: PrefixExpression, parent: Node): Iterable { - if(expr.operator == "not") { - // not(x) --> x==0 - // this means that "not" will never occur anywhere again in the ast after this - val dt = expr.expression.inferType(program).getOr(DataType.UBYTE) - val replacement = BinaryExpression(expr.expression, "==", NumericLiteral(dt,0.0, expr.position), expr.position) - return listOf(IAstModification.ReplaceNodeSafe(expr, replacement, parent)) - } - return noModifications - } - override fun after(decl: VarDecl, parent: Node): Iterable { val nextAssignment = decl.nextSibling() as? Assignment if(nextAssignment!=null && nextAssignment.origin!=AssignmentOrigin.VARINIT) { diff --git a/compiler/src/prog8/compiler/astprocessing/NotExpressionChanger.kt b/compiler/src/prog8/compiler/astprocessing/NotExpressionChanger.kt index 5ce666edc..b76a0211c 100644 --- a/compiler/src/prog8/compiler/astprocessing/NotExpressionChanger.kt +++ b/compiler/src/prog8/compiler/astprocessing/NotExpressionChanger.kt @@ -8,6 +8,7 @@ import prog8.ast.expressions.NumericLiteral import prog8.ast.expressions.PrefixExpression import prog8.ast.walk.AstWalker import prog8.ast.walk.IAstModification +import prog8.code.core.DataType import prog8.code.core.IErrorReporter import prog8.code.core.IntegerDatatypes @@ -39,54 +40,6 @@ internal class NotExpressionChanger(val program: Program, val errors: IErrorRepo } } } - - val left = expr.left as? BinaryExpression - val right = expr.right as? BinaryExpression - val leftValue = left?.right?.constValue(program)?.number - val rightValue = right?.right?.constValue(program)?.number - - if(expr.operator == "or") { - if(left?.operator=="==" && right?.operator=="==" && leftValue==0.0 && rightValue==0.0) { - // (a==0) or (b==0) -> (a and b)==0 - val orExpr = BinaryExpression(left.left, "and", right.left, expr.position) - val equalsZero = BinaryExpression(orExpr, "==", NumericLiteral.fromBoolean(false, expr.position), expr.position) - return listOf(IAstModification.ReplaceNode(expr, equalsZero, parent)) - } - } - else if(expr.operator == "and") { - if(left?.operator=="==" && right?.operator=="==" && leftValue==0.0 && rightValue==0.0) { - // (a==0) and (b==0) -> (a or b)==0 - val orExpr = BinaryExpression(left.left, "or", right.left, expr.position) - val equalsZero = BinaryExpression(orExpr, "==", NumericLiteral.fromBoolean(false, expr.position), expr.position) - return listOf(IAstModification.ReplaceNode(expr, equalsZero, parent)) - } - } - return noModifications - } - - override fun after(expr: BinaryExpression, parent: Node): Iterable { - // not a or not b -> not(a and b) - if(expr.operator=="or") { - val left = expr.left as? PrefixExpression - val right = expr.right as? PrefixExpression - if(left?.operator=="not" && right?.operator=="not") { - val andExpr = BinaryExpression(left.expression, "and", right.expression, expr.position) - val notExpr = PrefixExpression("not", andExpr, expr.position) - return listOf(IAstModification.ReplaceNode(expr, notExpr, parent)) - } - } - - // not a and not b -> not(a or b) - if(expr.operator=="and") { - val left = expr.left as? PrefixExpression - val right = expr.right as? PrefixExpression - if(left?.operator=="not" && right?.operator=="not") { - val andExpr = BinaryExpression(left.expression, "or", right.expression, expr.position) - val notExpr = PrefixExpression("not", andExpr, expr.position) - return listOf(IAstModification.ReplaceNode(expr, notExpr, parent)) - } - } - return noModifications } @@ -116,6 +69,12 @@ internal class NotExpressionChanger(val program: Program, val errors: IErrorRepo return listOf(IAstModification.ReplaceNode(expr, subBinExpr, parent)) } } + + // all other not(x) --> x==0 + // this means that "not" will never occur anywhere again in the ast after this + val dt = expr.expression.inferType(program).getOr(DataType.UBYTE) + val replacement = BinaryExpression(expr.expression, "==", NumericLiteral(dt,0.0, expr.position), expr.position) + return listOf(IAstModification.ReplaceNodeSafe(expr, replacement, parent)) } return noModifications } diff --git a/compiler/test/TestOptimization.kt b/compiler/test/TestOptimization.kt index 2a283d2bb..0ff41c9e6 100644 --- a/compiler/test/TestOptimization.kt +++ b/compiler/test/TestOptimization.kt @@ -251,15 +251,15 @@ class TestOptimization: FunSpec({ (initY2.value as NumericLiteral).number shouldBe 11.0 } - xtest("various 'not' operator rewrites even without optimizations on") { + test("various 'not' operator rewrites even without optimizations on") { val src = """ main { sub start() { ubyte a1 ubyte a2 a1 = not not a1 ; a1 = a1==0 - a1 = not a1 or not a2 ; a1 = (a1 and a2)==0 - a1 = not a1 and not a2 ; a1 = (a1 or a2)==0 + a1 = not a1 or not a2 ; a1 = a1==0 | a2==0 + a1 = not a1 and not a2 ; a1 = a1==0 & a2==0 } } """ @@ -273,20 +273,8 @@ class TestOptimization: FunSpec({ val value3 = (stmts[6] as Assignment).value as BinaryExpression value1.operator shouldBe "==" value1.right shouldBe NumericLiteral(DataType.UBYTE, 0.0, Position.DUMMY) - value2.operator shouldBe "==" - value2.right shouldBe NumericLiteral(DataType.UBYTE, 0.0, Position.DUMMY) - value3.operator shouldBe "==" - value3.right shouldBe NumericLiteral(DataType.UBYTE, 0.0, Position.DUMMY) - val left1 = value1.left as IdentifierReference - val left2 = value2.left as BinaryExpression - val left3 = value3.left as BinaryExpression - left1.nameInSource shouldBe listOf("a1") - left2.operator shouldBe "and" - (left2.left as IdentifierReference).nameInSource shouldBe listOf("a1") - (left2.right as IdentifierReference).nameInSource shouldBe listOf("a2") - left3.operator shouldBe "or" - (left3.left as IdentifierReference).nameInSource shouldBe listOf("a1") - (left3.right as IdentifierReference).nameInSource shouldBe listOf("a2") + value2.operator shouldBe "|" + value3.operator shouldBe "&" } test("intermediate assignment steps generated for typecasted expression") { diff --git a/docs/source/todo.rst b/docs/source/todo.rst index 430e250d0..65c3f48d1 100644 --- a/docs/source/todo.rst +++ b/docs/source/todo.rst @@ -3,8 +3,6 @@ TODO For next release ^^^^^^^^^^^^^^^^ -- re-enable unittest "various 'not' operator rewrites even without optimizations on" when not-problem is fixed - - petaxian roller.p8 line 49 (also see test.p8) generates large code compared to 8.2 - code gen for if statements has become inefficient? vm/6502? @@ -22,8 +20,6 @@ For next release - compiling logical.p8 to virtual with optimization generates a lot larger code as without optimizations. this is not the case for the 6502 codegen. -- bin expr splitter: split logical expressions on ands/ors/xors ? - - add some more optimizations in vmPeepholeOptimizer - vm Instruction needs to know what the read-registers/memory are, and what the write-register/memory is. this info is needed for more advanced optimizations and later code generation steps. diff --git a/examples/test.p8 b/examples/test.p8 index 7c9b7b556..4a1c37181 100644 --- a/examples/test.p8 +++ b/examples/test.p8 @@ -8,41 +8,39 @@ main { sub start() { ubyte a1 = 0 - ubyte a2 = 128 uword w1 = 0 + ubyte vv -; if not a1 and not w1 -; txt.print("1") -; if (0==a1) and (0==w1) -; txt.print("a") -; txt.nl() - a1 = 0 - w1 = 4096 - if not a1 and not w1 - txt.print("fail ") - else + if not a1 txt.print("ok ") - if (0==a1) and (0==w1) - txt.print("fail ") else - txt.print("ok ") + txt.print("fail ") + vv = not a1 + txt.print_ub(vv) txt.nl() - a1=128 - w1=2 - if not a1 and not w1 - txt.print("fail") - if (0==a1) and (0==w1) - txt.print("fail") + if not a1 + txt.print("fail ") + else + txt.print("ok ") + vv = not a1 + txt.print_ub(vv) txt.nl() - w1=2 - if not a1 and not w1 - txt.print("fail") - if (0==a1) and (0==w1) - txt.print("fail") + if not w1 + txt.print("ok ") + else + txt.print("fail ") + vv = not w1 + txt.print_ub(vv) + txt.nl() + w1 = 4096 + if not w1 + txt.print("fail ") + else + txt.print("ok ") + vv = not w1 + txt.print_ub(vv) txt.nl() - - ; petaxian roller.p8 line 49