From efe69eb5a9b129194fad90d105fd0e495bfd4a9b Mon Sep 17 00:00:00 2001 From: Karol Stasiak Date: Wed, 26 Jun 2019 22:17:46 +0200 Subject: [PATCH] Fix performance regressions and some bugs --- .../scala/millfork/assembly/z80/ZLine.scala | 17 +++ .../ByteVariableToRegisterOptimization.scala | 13 +- .../z80/opt/EmptyMemoryStoreRemoval.scala | 8 +- .../assembly/z80/opt/VariableLifetime.scala | 132 +++++++++++++++--- .../AbstractStatementPreprocessor.scala | 2 +- .../z80/Z80StatementPreprocessor.scala | 2 + .../scala/millfork/test/emu/EmuZ80Run.scala | 1 + 7 files changed, 149 insertions(+), 26 deletions(-) diff --git a/src/main/scala/millfork/assembly/z80/ZLine.scala b/src/main/scala/millfork/assembly/z80/ZLine.scala index 4be00443..61ad06dc 100644 --- a/src/main/scala/millfork/assembly/z80/ZLine.scala +++ b/src/main/scala/millfork/assembly/z80/ZLine.scala @@ -906,6 +906,23 @@ case class ZLine(opcode: ZOpcode.Value, registers: ZRegisters, parameter: Consta } } + def accessesMemoryViaGivenRegister(r: ZRegister.Value): Boolean = { + import ZRegister._ + import ZOpcode._ + registers match { + case TwoRegisters(MEM_HL, _) | TwoRegisters(_, MEM_HL) | OneRegister(MEM_HL) => r == MEM_HL + case TwoRegisters(MEM_BC, _) | TwoRegisters(_, MEM_BC) | OneRegister(MEM_BC) => r == MEM_BC + case TwoRegisters(MEM_DE, _) | TwoRegisters(_, MEM_DE) | OneRegister(MEM_DE) => r == MEM_DE + // TODO: IX and IY + case _ => opcode match { + case LD_HLIA | LD_HLDA | LD_AHLD | LD_AHLI => r == MEM_HL + case LHLX => r == MEM_DE + case SHLX => r == MEM_DE + case _ => false + } + } + } + def changesRegisterAndOffset(r: ZRegister.Value, o: Int): Boolean = { import ZOpcode._ import ZRegister._ diff --git a/src/main/scala/millfork/assembly/z80/opt/ByteVariableToRegisterOptimization.scala b/src/main/scala/millfork/assembly/z80/opt/ByteVariableToRegisterOptimization.scala index 05cf03ee..39351be8 100644 --- a/src/main/scala/millfork/assembly/z80/opt/ByteVariableToRegisterOptimization.scala +++ b/src/main/scala/millfork/assembly/z80/opt/ByteVariableToRegisterOptimization.scala @@ -241,11 +241,14 @@ object ByteVariableToRegisterOptimization extends AssemblyOptimization[ZLine] { def unapply(c: Int): Option[Int] = if ("IY+" + c == vname) Some(c) else None } code match { - case (_, ZLine0(LD_16, TwoRegisters(_, SP), _)) :: _ if vname.contains("+") => None - case (_, ZLine0(LD_16, TwoRegisters(SP, _), _)) :: _ if vname.contains("+") => None - case (_, ZLine0(ADD_16 | SBC_16, TwoRegisters(_, SP), _)) :: _ if vname.contains("+") => None - case (_, ZLine0(LD_HLSP | LD_DESP, _, _)) :: _ if vname.contains("+") => None - case (_, ZLine0(EX_SP, _, _)) :: _ if vname.contains("+") => None + case (_, ZLine0(LD_16, TwoRegisters(HL | IY, SP), _)) :: _ if vname.startsWith("IX+") => fail(121) + case (_, ZLine0(LD_16, TwoRegisters(HL | IX, SP), _)) :: _ if vname.startsWith("IY+") => fail(121) + case (_, ZLine0(LD_16, TwoRegisters(SP, HL | IY), _)) :: _ if vname.startsWith("IX+") => fail(122) + case (_, ZLine0(LD_16, TwoRegisters(SP, HL | IX), _)) :: _ if vname.startsWith("IY+") => fail(122) + case (_, ZLine0(ADD_16 | SBC_16, TwoRegisters(HL | IY, SP), _)) :: _ if vname.startsWith("IX+") => fail(123) + case (_, ZLine0(ADD_16 | SBC_16, TwoRegisters(HL | IX, SP), _)) :: _ if vname.startsWith("IY+") => fail(123) + case (_, ZLine0(LD_HLSP | LD_DESP, _, _)) :: _ if vname.contains("+") => fail(124) + case (_, ZLine0(EX_SP, _, _)) :: _ if vname.contains("+") => fail(125) case (_, ZLine0(LD, TwoRegisters(A, MEM_ABS_8), ThisVar(_))) :: xs => canBeInlined(vname, synced, target, addressInHl, addressInBc, addressInDe, xs).map(add(CyclesAndBytes(9, 2))) diff --git a/src/main/scala/millfork/assembly/z80/opt/EmptyMemoryStoreRemoval.scala b/src/main/scala/millfork/assembly/z80/opt/EmptyMemoryStoreRemoval.scala index 5b8312ae..16866d8c 100644 --- a/src/main/scala/millfork/assembly/z80/opt/EmptyMemoryStoreRemoval.scala +++ b/src/main/scala/millfork/assembly/z80/opt/EmptyMemoryStoreRemoval.scala @@ -31,6 +31,7 @@ object EmptyMemoryStoreRemoval extends AssemblyOptimization[ZLine] { val firstVariableAccess = code(firstaccess) val lastVariableAccess = code(lastaccess) import millfork.assembly.z80.ZOpcode._ + // TODO: this might be buggy; needs more testing. if ((firstVariableAccess match { case ZLine(LD, TwoRegisters(MEM_HL, _), _, Elidability.Elidable, _) => true case ZLine(LD | LD_16, TwoRegisters(MEM_ABS_8 | MEM_ABS_16, _), _, Elidability.Elidable, _) => true @@ -54,7 +55,12 @@ object EmptyMemoryStoreRemoval extends AssemblyOptimization[ZLine] { if (toRemove.isEmpty) { code } else { - optimizationContext.log.debug(s"Removing pointless store(s) to ${badVariables.mkString(", ")}") + optimizationContext.log.debug(s"Removing pointless store(s) to automatic variables ${badVariables.mkString(", ")}") +// val range = toRemove.min to toRemove.max +// range.map{ i => +// if (toRemove(i)) f"${code(i)}%-42s <-- REMOVE" +// else code(i).toString +// }.foreach(println) code.zipWithIndex.filter(x => !toRemove(x._2)).map(_._1) } } diff --git a/src/main/scala/millfork/assembly/z80/opt/VariableLifetime.scala b/src/main/scala/millfork/assembly/z80/opt/VariableLifetime.scala index 35f3cde8..bce9e94b 100644 --- a/src/main/scala/millfork/assembly/z80/opt/VariableLifetime.scala +++ b/src/main/scala/millfork/assembly/z80/opt/VariableLifetime.scala @@ -1,7 +1,7 @@ package millfork.assembly.z80.opt import millfork.assembly.opt.SingleStatus -import millfork.assembly.z80.{OneRegister, TwoRegisters, ZLine, ZLine0, ZOpcode} +import millfork.assembly.z80.{NoRegisters, OneRegister, TwoRegisters, ZLine, ZLine0, ZOpcode} import millfork.env._ import millfork.error.ConsoleLogger import millfork.node.ZRegister @@ -17,10 +17,7 @@ object VariableLifetime { import ZRegister._ import ZOpcode._ - val pointerLoadedAt = codeWithFlow.zipWithIndex.filter{ - case ((_, ZLine0(ZOpcode.LD_16, TwoRegisters(_, IMM_16), MemoryAddressConstant(MemoryVariable(n, _, _)))), _) => n == variableName - case _ => false - }.map(_._2) + val pointerLeaks: Boolean = isPointerLeaky(codeWithFlow, variableName) val pointerReadAt = codeWithFlow.zipWithIndex.filter{ case ((_, ZLine0(_, TwoRegisters(MEM_HL | MEM_DE | MEM_BC, _), _)), _) => true case ((_, ZLine0(_, TwoRegisters(_, MEM_HL | MEM_DE | MEM_BC), _)), _) => true @@ -42,12 +39,121 @@ object VariableLifetime { case _ => false }.toArray - if(pointerLoadedAt.nonEmpty) { + if(pointerLeaks) { pointerReadAt.foreach(i => flags(i) = true) } val code = codeWithFlow.map(_._2) - expandRangeToCoverLoops(code, flags) + val range = expandRangeToCoverLoops(code, flags) + +// val log = new ConsoleLogger +// log.verbosity = 3 +// log.trace("Lifetime for " + variableName) +// code.zipWithIndex.foreach { +// case (line, index) => +// if (index >= range.start && index < range.end) { +// log.trace(f"$line%-42s <") +// } else { +// log.trace(line.toString) +// } +// } + + range + } + + + def isPointerLeaky(codeWithFlow: List[(FlowInfo, ZLine)], variableName: String): Boolean = { + if (codeWithFlow.isEmpty) return false + var i = 0 + var inHl = false + var inBc = false + var inDe = false + import ZOpcode._ + import ZRegister._ + var previousFlow = codeWithFlow.head._1 + def fail(line: ZLine): Unit = { +// println(s"Pointer for variable $variableName leaks because of $line") + } + + for((flow, line) <- codeWithFlow) { + val imp = flow.importanceAfter + line match { + case ZLine0(ZOpcode.LD_16, TwoRegisters(HL, IMM_16), MemoryAddressConstant(MemoryVariable(n, _, _))) if n == variableName => + inHl = true + case ZLine0(ZOpcode.LD_16, TwoRegisters(BC, IMM_16), MemoryAddressConstant(MemoryVariable(n, _, _))) if n == variableName => + inBc = true + case ZLine0(ZOpcode.LD_16, TwoRegisters(DE, IMM_16), MemoryAddressConstant(MemoryVariable(n, _, _))) if n == variableName => + inDe = true + case ZLine0(ZOpcode.LD_16, TwoRegisters(HL, _), _) => inHl = false + case ZLine0(ZOpcode.LD_16, TwoRegisters(BC, _), _) => inBc = false + case ZLine0(ZOpcode.LD_16, TwoRegisters(DE, _), _) => inDe = false + case ZLine0(_, TwoRegisters(_, HL), _) if inHl => + fail(line) + return true + case ZLine0(_, TwoRegisters(_, BC), _) if inBc => + fail(line) + return true + case ZLine0(_, TwoRegisters(_, DE), _) if inDe => + fail(line) + return true + case ZLine0(LD_HLSP, _, _) => inHl = false + case ZLine0(LD_DESP, _, _) => inDe = false + case ZLine0(LHLX, _, _) if inHl => inHl = false + case ZLine0(SHLX, _, _) if inDe || inHl => + fail(line) + return false + case ZLine0(POP, OneRegister(HL), _) => inHl = false + case ZLine0(POP, OneRegister(BC), _) => inBc = false + case ZLine0(POP, OneRegister(DE), _) => inDe = false + case ZLine0(PUSH, OneRegister(HL), _) if inHl => + fail(line) + return true + case ZLine0(PUSH, OneRegister(BC), _) if inBc => + fail(line) + return true + case ZLine0(PUSH, OneRegister(DE), _) if inDe => + fail(line) + return true + case ZLine0(RET | RETI | RETN | JP | JR, NoRegisters, _) => + inHl = false + inBc = false + inDe = false + case ZLine0(LABEL, _, _) if inHl && (imp.h == Important || imp.l == Important) => + fail(line) + return true + case ZLine0(LABEL, _, _) if inBc && (imp.b == Important || imp.c == Important) => + fail(line) + return true + case ZLine0(LABEL, _, _) if inDe && (imp.d == Important || imp.e == Important) => + fail(line) + return true + case _ if !line.accessesMemoryViaGivenRegister(MEM_HL) && line.readsRegister(H) && inHl => + fail(line) + return true + case _ if !line.accessesMemoryViaGivenRegister(MEM_HL) && line.readsRegister(L) && inHl => + fail(line) + return true + case _ if !line.accessesMemoryViaGivenRegister(MEM_BC) && line.readsRegister(B) && inBc => + fail(line) + return true + case _ if !line.accessesMemoryViaGivenRegister(MEM_BC) && line.readsRegister(C) && inBc => + fail(line) + return true + case _ if !line.accessesMemoryViaGivenRegister(MEM_DE) && line.readsRegister(D) && inDe => + fail(line) + return true + case _ if !line.accessesMemoryViaGivenRegister(MEM_DE) && line.readsRegister(E) && inDe => + fail(line) + return true + case ZLine0(LD, TwoRegisters(H | L, _), _) => inHl = false + case ZLine0(LD, TwoRegisters(B | C, _), _) => inBc = false + case ZLine0(LD, TwoRegisters(D | E, _), _) => inDe = false + case _ => // TODO: ??? + } + i += 1 + previousFlow = flow + } + false } def expandRangeToCoverLoops(code: List[ZLine], flags: Array[Boolean]): Range = { @@ -76,18 +182,6 @@ object VariableLifetime { } } -// val log = new ConsoleLogger -// log.verbosity = 3 -// log.trace("Lifetime for " + variableName) -// codeWithFlow.zipWithIndex.foreach { -// case ((_, line), index) => -// if (index >= min && index < max) { -// log.trace(f"$line%-30s <") -// } else { -// log.trace(line.toString) -// } -// } - Range(min, max) } } diff --git a/src/main/scala/millfork/compiler/AbstractStatementPreprocessor.scala b/src/main/scala/millfork/compiler/AbstractStatementPreprocessor.scala index 8fc5ea73..b01856a4 100644 --- a/src/main/scala/millfork/compiler/AbstractStatementPreprocessor.scala +++ b/src/main/scala/millfork/compiler/AbstractStatementPreprocessor.scala @@ -12,7 +12,7 @@ import scala.collection.mutable.ListBuffer /** * @author Karol Stasiak */ -abstract class AbstractStatementPreprocessor(ctx: CompilationContext, statements: List[ExecutableStatement]) { +abstract class AbstractStatementPreprocessor(protected val ctx: CompilationContext, statements: List[ExecutableStatement]) { type VV = Map[String, Constant] protected val optimize = true // TODO protected val env: Environment = ctx.env diff --git a/src/main/scala/millfork/compiler/z80/Z80StatementPreprocessor.scala b/src/main/scala/millfork/compiler/z80/Z80StatementPreprocessor.scala index fc3c9972..610451fe 100644 --- a/src/main/scala/millfork/compiler/z80/Z80StatementPreprocessor.scala +++ b/src/main/scala/millfork/compiler/z80/Z80StatementPreprocessor.scala @@ -1,5 +1,6 @@ package millfork.compiler.z80 +import millfork.CompilationFlag import millfork.compiler.{AbstractStatementPreprocessor, CompilationContext} import millfork.env.{MemoryVariable, MfArray, PointerType, Thing, Variable, VariableAllocationMethod} import millfork.node.{Assignment, DerefDebuggingExpression, DoWhileStatement, ExecutableStatement, Expression, ExpressionStatement, ForDirection, ForEachStatement, ForStatement, FunctionCallExpression, IfStatement, IndexedExpression, IndirectFieldExpression, LhsExpression, LiteralExpression, Node, Statement, SumExpression, VariableDeclarationStatement, VariableExpression, WhileStatement} @@ -38,6 +39,7 @@ class Z80StatementPreprocessor(ctx: CompilationContext, statements: List[Executa def maybeOptimizeForStatement(f: ForStatement): Option[(ExecutableStatement, VV)] = { + if (!ctx.options.flag(CompilationFlag.DangerousOptimizations)) return None // TODO: figure out when this is useful // Currently all instances of arr[i] are replaced with arr`popt`i[0], where arr`popt`i is a new pointer variable. // This breaks the main Millfork promise of not using hidden variables! diff --git a/src/test/scala/millfork/test/emu/EmuZ80Run.scala b/src/test/scala/millfork/test/emu/EmuZ80Run.scala index 23e6a6f8..dbf35d90 100644 --- a/src/test/scala/millfork/test/emu/EmuZ80Run.scala +++ b/src/test/scala/millfork/test/emu/EmuZ80Run.scala @@ -77,6 +77,7 @@ class EmuZ80Run(cpu: millfork.Cpu.Value, nodeOptimizations: List[NodeOptimizatio println(source) val platform = EmuPlatform.get(cpu) val extraFlags = Map( + CompilationFlag.DangerousOptimizations -> true, CompilationFlag.EnableInternalTestSyntax -> true, CompilationFlag.InlineFunctions -> this.inline, CompilationFlag.OptimizeStdlib -> this.inline,