Important optimization bugfixes + empty memory store removal

src/main/scala/millfork/NonOverlappingIntervals.scala

@@ -0,0 +1,24 @@
+package millfork
+
+import scala.collection.mutable.ListBuffer
+
+/**
+  * @author Karol Stasiak
+  */
+object NonOverlappingIntervals {
+  def apply[T](intervals: Iterable[T], start: T => Int, end: T => Int): Seq[Set[T]] = {
+    val builder = ListBuffer[Set[T]]()
+
+    def scan(set: Set[T], lastEnd: Int): Unit = {
+      builder += set
+      for (interval <- intervals) {
+        if (start(interval) >= lastEnd) {
+          scan(set + interval, end(interval))
+        }
+      }
+    }
+
+    scan(Set(), 0)
+    builder.toIndexedSeq
+  }
+}

src/main/scala/millfork/OptimizationPresets.scala

@@ -53,7 +53,7 @@ object OptimizationPresets {
     LaterOptimizations.DoubleLoadToTheSameRegister,
     LaterOptimizations.DoubleLoadToDifferentRegisters,
     LaterOptimizations.DoubleLoadToTheSameRegister,
-    AlwaysGoodOptimizations.PointlessStoreAfterLoad,
+    EmptyMemoryStoreRemoval,
     AlwaysGoodOptimizations.PoinlessLoadBeforeAnotherLoad,
     AlwaysGoodOptimizations.IdempotentDuplicateRemoval,
     AlwaysGoodOptimizations.ConstantIndexPropagation,
@@ -64,7 +64,7 @@ object OptimizationPresets {
     AlwaysGoodOptimizations.PointlessRegisterTransfersBeforeStore,
     AlwaysGoodOptimizations.PointlessStashingToIndexOverShortSafeBranch,
     AlwaysGoodOptimizations.RearrangeMath,
-    AlwaysGoodOptimizations.PointlessStoreAfterLoad,
+    EmptyMemoryStoreRemoval,
     AlwaysGoodOptimizations.PointlessLoadBeforeReturn,
     LaterOptimizations.PointessLoadingForShifting,
     AlwaysGoodOptimizations.SimplifiableBitOpsSequence,
@@ -73,19 +73,19 @@ object OptimizationPresets {
     AlwaysGoodOptimizations.SimplifiableBitOpsSequence,
 
     LaterOptimizations.LoadingAfterShifting,
-    AlwaysGoodOptimizations.PointlessStoreAfterLoad,
+    EmptyMemoryStoreRemoval,
     AlwaysGoodOptimizations.PoinlessStoreBeforeStore,
     LaterOptimizations.PointlessLoadAfterStore,
     AlwaysGoodOptimizations.PoinlessLoadBeforeAnotherLoad,
 
     LaterOptimizations.LoadingAfterShifting,
-    AlwaysGoodOptimizations.PointlessStoreAfterLoad,
+    EmptyMemoryStoreRemoval,
     AlwaysGoodOptimizations.PoinlessStoreBeforeStore,
     LaterOptimizations.PointlessLoadAfterStore,
     AlwaysGoodOptimizations.PoinlessLoadBeforeAnotherLoad,
 
     LaterOptimizations.LoadingAfterShifting,
-    AlwaysGoodOptimizations.PointlessStoreAfterLoad,
+    EmptyMemoryStoreRemoval,
     AlwaysGoodOptimizations.PoinlessStoreBeforeStore,
     LaterOptimizations.PointlessLoadAfterStore,
     AlwaysGoodOptimizations.PoinlessLoadBeforeAnotherLoad,
@@ -118,6 +118,7 @@ object OptimizationPresets {
     AlwaysGoodOptimizations.CommonBranchBodyOptimization,
     AlwaysGoodOptimizations.ConstantFlowAnalysis,
     AlwaysGoodOptimizations.ConstantIndexPropagation,
+    EmptyMemoryStoreRemoval,
     AlwaysGoodOptimizations.FlagFlowAnalysis,
     AlwaysGoodOptimizations.IdempotentDuplicateRemoval,
     AlwaysGoodOptimizations.ImpossibleBranchRemoval,
@@ -137,7 +138,6 @@ object OptimizationPresets {
     AlwaysGoodOptimizations.PointlessRegisterTransfersBeforeCompare,
     AlwaysGoodOptimizations.PointlessRegisterTransfersBeforeReturn,
     AlwaysGoodOptimizations.PointlessStashingToIndexOverShortSafeBranch,
-    AlwaysGoodOptimizations.PointlessStoreAfterLoad,
     AlwaysGoodOptimizations.PoinlessStoreBeforeStore,
     AlwaysGoodOptimizations.RearrangeMath,
     AlwaysGoodOptimizations.RemoveNops,

src/main/scala/millfork/assembly/opt/AlwaysGoodOptimizations.scala

@@ -18,7 +18,7 @@ object AlwaysGoodOptimizations {
 
   val counter = new AtomicInteger(30000)
 
-  def getNextLabel(prefix: String) = f".${prefix}%s__${counter.getAndIncrement()}%05d"
+  def getNextLabel(prefix: String) = f".$prefix%s__${counter.getAndIncrement()}%05d"
 
   val PointlessMath = new RuleBasedAssemblyOptimization("Pointless math",
     needsFlowInfo = FlowInfoRequirement.NoRequirement,
@@ -138,19 +138,6 @@ object AlwaysGoodOptimizations {
       (HasOpcode(EOR) & HasAddrModeIn(Set(ZeroPage, Absolute)) & MatchParameter(0) & Elidable) ~~> (code => code.init :+ AssemblyLine.immediate(LDA, 0)),
   )
 
-  val PointlessStoreAfterLoad = new RuleBasedAssemblyOptimization("Pointless store after load",
-    needsFlowInfo = FlowInfoRequirement.NoRequirement,
-    (HasOpcode(LDA) & MatchAddrMode(0) & MatchParameter(1)) ~
-      (LinearOrLabel & DoesntChangeMemoryAt(0,1) & Not(ChangesA) & DoesntChangeIndexingInAddrMode(0)).* ~
-      (Elidable & HasOpcode(STA) & MatchAddrMode(0) & MatchParameter(1)) ~~> (_.init),
-    (HasOpcode(LDX) & MatchAddrMode(0) & MatchParameter(1)) ~
-      (LinearOrLabel & DoesntChangeMemoryAt(0,1) & Not(ChangesA) & DoesntChangeIndexingInAddrMode(0)).* ~
-      (Elidable & HasOpcode(STX) & MatchAddrMode(0) & MatchParameter(1)) ~~> (_.init),
-    (HasOpcode(LDY) & MatchAddrMode(0) & MatchParameter(1)) ~
-      (LinearOrLabel & DoesntChangeMemoryAt(0,1) & Not(ChangesA) & DoesntChangeIndexingInAddrMode(0)).* ~
-      (Elidable & HasOpcode(STY) & MatchAddrMode(0) & MatchParameter(1)) ~~> (_.init),
-  )
-
   val PoinlessStoreBeforeStore = new RuleBasedAssemblyOptimization("Pointless store before store",
     needsFlowInfo = FlowInfoRequirement.NoRequirement,
     (Elidable & HasAddrModeIn(Set(Absolute, ZeroPage)) & MatchParameter(1) & MatchAddrMode(2) & Set(STA, SAX, STX, STY, STZ)) ~
@@ -588,17 +575,17 @@ object AlwaysGoodOptimizations {
   val PointlessLoadAfterLoadOrStore = new RuleBasedAssemblyOptimization("Pointless load after load or store",
     needsFlowInfo = FlowInfoRequirement.NoRequirement,
 
-    (HasOpcodeIn(Set(LDA, STA)) & HasAddrMode(Implied) & MatchParameter(1)) ~
+    (HasOpcodeIn(Set(LDA, STA)) & HasAddrMode(Immediate) & MatchParameter(1)) ~
       (Linear & Not(ChangesA)).* ~
-      (Elidable & HasOpcode(LDA) & HasAddrMode(Implied) & MatchParameter(1)) ~~> (_.init),
+      (Elidable & HasOpcode(LDA) & HasAddrMode(Immediate) & MatchParameter(1)) ~~> (_.init),
 
-    (HasOpcodeIn(Set(LDX, STX)) & HasAddrMode(Implied) & MatchParameter(1)) ~
+    (HasOpcodeIn(Set(LDX, STX)) & HasAddrMode(Immediate) & MatchParameter(1)) ~
       (Linear & Not(ChangesX)).* ~
-      (Elidable & HasOpcode(LDX) & HasAddrMode(Implied) & MatchParameter(1)) ~~> (_.init),
+      (Elidable & HasOpcode(LDX) & HasAddrMode(Immediate) & MatchParameter(1)) ~~> (_.init),
 
-    (HasOpcodeIn(Set(LDY, STY)) & HasAddrMode(Implied) & MatchParameter(1)) ~
+    (HasOpcodeIn(Set(LDY, STY)) & HasAddrMode(Immediate) & MatchParameter(1)) ~
       (Linear & Not(ChangesY)).* ~
-      (Elidable & HasOpcode(LDY) & HasAddrMode(Implied) & MatchParameter(1)) ~~> (_.init),
+      (Elidable & HasOpcode(LDY) & HasAddrMode(Immediate) & MatchParameter(1)) ~~> (_.init),
 
     (HasOpcodeIn(Set(LDA, STA)) & MatchAddrMode(0) & MatchParameter(1)) ~
       (Linear & Not(ChangesA) & DoesntChangeIndexingInAddrMode(0) & DoesntChangeMemoryAt(0, 1)).* ~

src/main/scala/millfork/assembly/opt/EmptyMemoryStoreRemoval.scala

@@ -0,0 +1,72 @@
+package millfork.assembly.opt
+
+import millfork.CompilationOptions
+import millfork.assembly.AssemblyLine
+import millfork.env._
+import millfork.assembly.Opcode._
+import millfork.error.ErrorReporting
+
+import scala.collection.{immutable, mutable}
+
+/**
+  * @author Karol Stasiak
+  */
+object EmptyMemoryStoreRemoval extends AssemblyOptimization {
+  override def name = "Removing pointless stores to automatic variables"
+
+  private val storeOpcodes = Set(STA, STX, STY, STZ, SAX)
+
+  override def optimize(f: NormalFunction, code: List[AssemblyLine], options: CompilationOptions): List[AssemblyLine] = {
+    val paramVariables = f.params match {
+      case NormalParamSignature(ps) =>
+        ps.map(_.name).toSet
+      case _ =>
+        // assembly functions do not get this optimization
+        return code
+    }
+    val stillUsedVariables = code.flatMap {
+      case AssemblyLine(_, _, MemoryAddressConstant(th), _) => Some(th.name)
+      case _ => None
+    }.toSet
+    val localVariables = f.environment.getAllLocalVariables.filter {
+      case MemoryVariable(name, typ, VariableAllocationMethod.Auto) =>
+        typ.size == 1 && !paramVariables(name) && stillUsedVariables(name)
+      case _ => false
+    }
+
+    if (localVariables.isEmpty) {
+      return code
+    }
+
+    val toRemove = mutable.Set[Int]()
+    val badVariables = mutable.Set[String]()
+    var importances: immutable.Seq[CpuImportance] = null
+
+    for(v <- localVariables) {
+      val lifetime = VariableLifetime.apply(v.name, code)
+      val lastaccess = lifetime.last
+      if (lastaccess >= 0) {
+        if (code(lastaccess).opcode match {
+          case  STA | STX | STY | SAX =>
+            true
+          case INC | DEC =>
+            if (importances eq null) {
+              importances = ReverseFlowAnalyzer.analyze(f, code)
+            }
+            importances(lastaccess).z != Important && importances(lastaccess).n != Important
+          case _ => // last variable access is important, or we're in a loop, do not remove
+            false
+        }) {
+          badVariables += v.name
+          toRemove += lastaccess
+        }
+      }
+    }
+    if (toRemove.isEmpty) {
+      code
+    } else {
+      ErrorReporting.debug(s"Removing pointless store(s) to ${badVariables.mkString(", ")}")
+      code.zipWithIndex.filter(x => !toRemove(x._2)).map(_._1)
+    }
+  }
+}

src/main/scala/millfork/assembly/opt/LaterOptimizations.scala

@@ -73,9 +73,9 @@ object LaterOptimizations {
   //noinspection ZeroIndexToHead
   private def InterleavedImmediateLoads(load: Opcode.Value, store: Opcode.Value) = {
     (Elidable & HasOpcode(load) & MatchImmediate(0)) ~
-      (Elidable & HasOpcode(store) & HasAddrMode(Absolute) & MatchParameter(8)) ~
+      (Elidable & HasOpcode(store) & HasAddrModeIn(Set(Absolute, ZeroPage)) & MatchParameter(8)) ~
       (Elidable & HasOpcode(load) & MatchImmediate(1)) ~
-      (Elidable & HasOpcode(store) & HasAddrMode(Absolute) & MatchParameter(9) & DontMatchParameter(8)) ~
+      (Elidable & HasOpcode(store) & HasAddrModeIn(Set(Absolute, ZeroPage)) & MatchParameter(9) & DontMatchParameter(8)) ~
       (Elidable & HasOpcode(load) & MatchImmediate(0)) ~~> { c =>
       List(c(2), c(3), c(0), c(1))
     }
@@ -83,11 +83,11 @@ object LaterOptimizations {
 
   //noinspection ZeroIndexToHead
   private def InterleavedAbsoluteLoads(load: Opcode.Value, store: Opcode.Value) = {
-    (Elidable & HasOpcode(load) & HasAddrMode(Absolute) & MatchParameter(0)) ~
-      (Elidable & HasOpcode(store) & HasAddrMode(Absolute) & MatchParameter(8) & DontMatchParameter(0)) ~
-      (Elidable & HasOpcode(load) & HasAddrMode(Absolute) & MatchParameter(1) & DontMatchParameter(8) & DontMatchParameter(0)) ~
-      (Elidable & HasOpcode(store) & HasAddrMode(Absolute) & MatchParameter(9) & DontMatchParameter(8) & DontMatchParameter(1) & DontMatchParameter(0)) ~
-      (Elidable & HasOpcode(load) & HasAddrMode(Absolute) & MatchParameter(0)) ~~> { c =>
+    (Elidable & HasOpcode(load) & HasAddrModeIn(Set(Absolute, ZeroPage)) & MatchParameter(0)) ~
+      (Elidable & HasOpcode(store) & HasAddrModeIn(Set(Absolute, ZeroPage)) & MatchParameter(8) & DontMatchParameter(0)) ~
+      (Elidable & HasOpcode(load) & HasAddrModeIn(Set(Absolute, ZeroPage)) & MatchParameter(1) & DontMatchParameter(8) & DontMatchParameter(0)) ~
+      (Elidable & HasOpcode(store) & HasAddrModeIn(Set(Absolute, ZeroPage)) & MatchParameter(9) & DontMatchParameter(8) & DontMatchParameter(1) & DontMatchParameter(0)) ~
+      (Elidable & HasOpcode(load) & HasAddrModeIn(Set(Absolute, ZeroPage)) & MatchParameter(0)) ~~> { c =>
       List(c(2), c(3), c(0), c(1))
     }
   }

src/main/scala/millfork/assembly/opt/VariableLifetime.scala

@@ -0,0 +1,56 @@
+package millfork.assembly.opt
+
+import millfork.assembly.AssemblyLine
+import millfork.env._
+import millfork.error.ErrorReporting
+
+/**
+  * @author Karol Stasiak
+  */
+object VariableLifetime {
+
+  // TODO: This only works for 1-byte non-stack variables.
+  // Should be fixed to also work with larger variables.
+  def apply(variableName: String, code: List[AssemblyLine]): Range = {
+    val flags = code.map(_.parameter match {
+      case MemoryAddressConstant(MemoryVariable(n, _, _)) if n == variableName => true
+      case _ => false
+    })
+    if (flags.forall(!_)) return Range(0, 0)
+    var min = flags.indexOf(true)
+    var max = flags.lastIndexOf(true) + 1
+    var changed = true
+    val labelMap = code.zipWithIndex.flatMap(a => a._1.parameter match {
+      case MemoryAddressConstant(Label(l)) => List(l -> a._2)
+      case _ => Nil
+    }).groupBy(_._1).mapValues(_.map(_._2).toSet)
+
+    while (changed) {
+      changed = false
+      for ((label, indices) <- labelMap) {
+        if (indices.exists(i => i >= min && i < max)) {
+          indices.foreach { i =>
+            val before = max - min
+            min = min min i
+            max = max max (i + 1)
+            if (max - min != before) {
+              changed = true
+            }
+          }
+        }
+      }
+    }
+
+//    ErrorReporting.trace("Lifetime for " + variableName)
+//    code.zipWithIndex.foreach {
+//      case (line, index) =>
+//        if (index >= min && index < max) {
+//          ErrorReporting.trace(f"$line%-30s  <")
+//        } else {
+//          ErrorReporting.trace(line.toString)
+//        }
+//    }
+
+    Range(min, max)
+  }
+}

src/main/scala/millfork/env/Constant.scala

@@ -60,9 +60,13 @@ sealed trait Constant {
   def isLowestByteAlwaysEqual(i: Int) : Boolean = false
 
   def quickSimplify: Constant = this
+
+  def isRelatedTo(v: Variable): Boolean
 }
 
-case class UnexpandedConstant(name: String, requiredSize: Int) extends Constant
+case class UnexpandedConstant(name: String, requiredSize: Int) extends Constant {
+  override def isRelatedTo(v: Variable): Boolean = false
+}
 
 case class NumericConstant(value: Long, requiredSize: Int) extends Constant {
   if (requiredSize == 1) {
@@ -80,12 +84,16 @@ case class NumericConstant(value: Long, requiredSize: Int) extends Constant {
   override def +(that: Long) = NumericConstant(value + that, minimumSize(value + that))
 
   override def toString: String = if (value > 9) value.formatted("$%X") else value.toString
+
+  override def isRelatedTo(v: Variable): Boolean = false
 }
 
 case class MemoryAddressConstant(var thing: ThingInMemory) extends Constant {
   override def requiredSize = 2
 
   override def toString: String = thing.name
+
+  override def isRelatedTo(v: Variable): Boolean = thing.name == v.name
 }
 
 case class HalfWordConstant(base: Constant, hi: Boolean) extends Constant {
@@ -104,6 +112,8 @@ case class HalfWordConstant(base: Constant, hi: Boolean) extends Constant {
   override def requiredSize = 1
 
   override def toString: String = base + (if (hi) ".hi" else ".lo")
+
+  override def isRelatedTo(v: Variable): Boolean = base.isRelatedTo(v)
 }
 
 case class SubbyteConstant(base: Constant, index: Int) extends Constant {
@@ -127,6 +137,8 @@ case class SubbyteConstant(base: Constant, index: Int) extends Constant {
     case 2 => ".b2"
     case 3 => ".b3"
   })
+
+  override def isRelatedTo(v: Variable): Boolean = base.isRelatedTo(v)
 }
 
 object MathOperator extends Enumeration {
@@ -221,4 +233,6 @@ case class CompoundConstant(operator: MathOperator.Value, lhs: Constant, rhs: Co
   }
 
   override def requiredSize: Int = lhs.requiredSize max rhs.requiredSize
+
+  override def isRelatedTo(v: Variable): Boolean = lhs.isRelatedTo(v) || rhs.isRelatedTo(v)
 }

src/test/scala/millfork/test/AssemblyOptimizationSuite.scala

@@ -172,7 +172,6 @@ class AssemblyOptimizationSuite extends FunSuite with Matchers {
   test("TAX-BCC-RTS-TXA optimization") {
     new EmuRun(Cpu.StrictMos,
       OptimizationPresets.NodeOpt, List(
-        AlwaysGoodOptimizations.PointlessStoreAfterLoad,
         LaterOptimizations.PointlessLoadAfterStore,
         VariableToRegisterOptimization,
         LaterOptimizations.DoubleLoadToDifferentRegisters,

src/test/scala/millfork/test/auxilary/NonOverlappingIntervalsTest.scala

@@ -0,0 +1,16 @@
+package millfork.test.auxilary
+
+import millfork.NonOverlappingIntervals
+
+/**
+  * @author Karol Stasiak
+  */
+object NonOverlappingIntervalsTest {
+  def main(args: Array[String]): Unit = {
+    NonOverlappingIntervals.apply[(Int, Int)](
+      List(0 -> 3, 1 -> 2, 2 -> 3, 0 -> 2, 1 -> 4),
+      _._1,
+      _._2
+    ).map(_.toSeq.sorted).foreach(println)
+  }
+}