From 09294307fdd882d86ac4a70eff7b143809414cb9 Mon Sep 17 00:00:00 2001 From: Karol Stasiak Date: Tue, 22 Oct 2019 00:41:34 +0200 Subject: [PATCH] #8 Standardise the behaviour of `for` loops. --- docs/abi/undefined-behaviour.md | 3 + docs/lang/syntax.md | 17 +- .../compiler/AbstractStatementCompiler.scala | 32 ++-- .../millfork/test/ForLoopSecondSuite.scala | 163 ++++++++++++++++++ .../scala/millfork/test/ForLoopSuite.scala | 14 +- 5 files changed, 202 insertions(+), 27 deletions(-) create mode 100644 src/test/scala/millfork/test/ForLoopSecondSuite.scala diff --git a/docs/abi/undefined-behaviour.md b/docs/abi/undefined-behaviour.md index bed412f4..3d384378 100644 --- a/docs/abi/undefined-behaviour.md +++ b/docs/abi/undefined-behaviour.md @@ -39,6 +39,9 @@ Currently, such functions may be evaluated either once or twice. This might be f * when using modifying operators: calling functions on the right-hand-side index expression than modify any of the variables used on the left hand side +* when using `for` loops operators: calling non-pure functions in the range limits (like in `for i,f(),to,g()`). +Currently, such functions may be evaluated any number of times. This might be fixed in the future. + * jumping across the scope of for loop that uses a fixed list or across functions * division by zero and modulo by zero diff --git a/docs/lang/syntax.md b/docs/lang/syntax.md index c1f9385f..c9561491 100644 --- a/docs/lang/syntax.md +++ b/docs/lang/syntax.md @@ -372,7 +372,7 @@ for : [ ] { * `` – an already defined numeric variable -* `` – the range to traverse: +* `` – the type of range to traverse: * `to` – from `` inclusive to `` inclusive, in ascending order (e.g. `0,to,9` to traverse 0, 1,... 9) @@ -388,6 +388,21 @@ for : [ ] { * `paralleluntil` – the same as `until`, but the iterations may be executed in any order There is no `paralleldownto`, because it would do the same as `parallelto` with swapped arguments. + + If: + + * the left argument to `until`, `paralleluntil`, `to` or `parallelto` is greater than the right argument + + * the left argument to `downto` is smaller than the right argument + + then the loop counter overflows and wraps around. + For example, `for i,254,to,1` with `i` being a byte, iterates over 254, 255, 0, 1. + + If the arguments to `until` or `paralleluntil` are equal, zero iterations are executed. + + `` and `` may be evaluated an arbitrary number of times. + It's recommended to use only constants, variables or other really simple expressions. + * `` – traverse enum constants of given type, in arbitrary order diff --git a/src/main/scala/millfork/compiler/AbstractStatementCompiler.scala b/src/main/scala/millfork/compiler/AbstractStatementCompiler.scala index 0e04f37f..69fd0245 100644 --- a/src/main/scala/millfork/compiler/AbstractStatementCompiler.scala +++ b/src/main/scala/millfork/compiler/AbstractStatementCompiler.scala @@ -148,7 +148,9 @@ abstract class AbstractStatementCompiler[T <: AbstractCode] { endEvaluated.foreach { value => val max = f.direction match { case ForDirection.To | ForDirection.ParallelTo | ForDirection.DownTo => value - case ForDirection.Until | ForDirection.ParallelUntil => value - 1 + case ForDirection.Until | ForDirection.ParallelUntil => + // dirty hack: + if (value.quickSimplify.isProvablyZero) value else value - 1 case _ => Constant.Zero } if (!max.quickSimplify.fitsInto(v.typ)) { @@ -162,15 +164,13 @@ abstract class AbstractStatementCompiler[T <: AbstractCode] { val end = ctx.nextLabel("of") val (main, extra) = compile(ctx.addLabels(names, Label(end), Label(end)), Assignment(vex, f.start).pos(p) :: f.body) main ++ labelChunk(end) -> extra - case (ForDirection.Until | ForDirection.ParallelUntil, Some(NumericConstant(s, ssize)), Some(NumericConstant(e, _))) if s >= e => + case (ForDirection.Until | ForDirection.ParallelUntil, Some(NumericConstant(s, ssize)), Some(NumericConstant(e, _))) if s == e => Nil -> Nil case (ForDirection.To | ForDirection.ParallelTo, Some(NumericConstant(s, ssize)), Some(NumericConstant(e, _))) if s == e => val end = ctx.nextLabel("of") val (main, extra) = compile(ctx.addLabels(names, Label(end), Label(end)), Assignment(vex, f.start).pos(p) :: f.body) main ++ labelChunk(end) -> extra - case (ForDirection.To | ForDirection.ParallelTo, Some(NumericConstant(s, ssize)), Some(NumericConstant(e, _))) if s > e => - Nil -> Nil case (ForDirection.Until | ForDirection.ParallelUntil, Some(c), Some(NumericConstant(256, _))) if variable.map(_.typ.size).contains(1) && c.requiredSize == 1 && c.isProvablyNonnegative => @@ -200,8 +200,6 @@ abstract class AbstractStatementCompiler[T <: AbstractCode] { val end = ctx.nextLabel("of") val (main, extra) = compile(ctx.addLabels(names, Label(end), Label(end)), Assignment(vex, LiteralExpression(s, ssize)).pos(p) :: f.body) main ++ labelChunk(end) -> extra - case (ForDirection.DownTo, Some(NumericConstant(s, ssize)), Some(NumericConstant(e, esize))) if s < e => - Nil -> Nil case (ForDirection.DownTo, Some(NumericConstant(s, 1)), Some(NumericConstant(0, _))) if s > 0 => compile(ctx, List( Assignment( @@ -219,10 +217,7 @@ abstract class AbstractStatementCompiler[T <: AbstractCode] { compile(ctx, List( Assignment( vex, - SumExpression( - List(false -> f.start, false -> LiteralExpression(1, 1).pos(p)), - decimal = false - ).pos(p) + f.start #-# 1 ).pos(p), DoWhileStatement( decrement :: f.body, @@ -237,7 +232,7 @@ abstract class AbstractStatementCompiler[T <: AbstractCode] { compile(ctx, List( Assignment(vex, f.start).pos(p), WhileStatement( - FunctionCallExpression("<", List(vex, f.end)).pos(p), + FunctionCallExpression("!=", List(vex, f.end)).pos(p), f.body, List(increment), names).pos(p) )) // case (ForDirection.To | ForDirection.ParallelTo, _, Some(NumericConstant(n, _))) if n > 0 && n < 255 => @@ -265,15 +260,12 @@ abstract class AbstractStatementCompiler[T <: AbstractCode] { val endMinusOne = (f.end #-# 1).pos(p) compile(ctx, List( Assignment(vex, f.start).pos(p), - IfStatement( - FunctionCallExpression(">=", List(vex, f.end)).pos(p), - List(DoWhileStatement( - f.body, - List(decrement), - FunctionCallExpression("!=", List(vex, endMinusOne)).pos(p), - names - ).pos(p)), - Nil) + DoWhileStatement( + f.body, + List(decrement), + FunctionCallExpression("!=", List(vex, endMinusOne)).pos(p), + names + ).pos(p) )) } } diff --git a/src/test/scala/millfork/test/ForLoopSecondSuite.scala b/src/test/scala/millfork/test/ForLoopSecondSuite.scala new file mode 100644 index 00000000..4811fb46 --- /dev/null +++ b/src/test/scala/millfork/test/ForLoopSecondSuite.scala @@ -0,0 +1,163 @@ +package millfork.test + +import millfork.Cpu +import millfork.test.emu.EmuUnoptimizedCrossPlatformRun +import org.scalactic.Prettifier +import org.scalatest.{AppendedClues, FunSuite, Matchers} + +/** + * @author Karol Stasiak + */ +class ForLoopSecondSuite extends FunSuite with Matchers with AppendedClues { + + sealed trait IterationBound + + case class ViaVar(i: Int) extends IterationBound { + override def toString: String = s"v(=$i)" + } + + case class ViaConst(i: Int) extends IterationBound { + override def toString: String = i.toString + } + + + def subcase(typ: String, from: IterationBound, dir: String, to: IterationBound, expectedCount: Int)(platforms: Cpu.Value*)(implicit pos: org.scalactic.source.Position, prettifier: Prettifier): Unit = { + val params = (from, to) match { + case (ViaVar(s), ViaVar(e)) => s"$typ s, $typ e" + case (ViaVar(s), ViaConst(e)) => s"$typ s" + case (ViaConst(s), ViaVar(e)) => s"$typ e" + case (ViaConst(s), ViaConst(e)) => "" + } + val args = (from, to) match { + case (ViaVar(s), ViaVar(e)) => s"$s, $e" + case (ViaVar(s), ViaConst(_)) => s"$s" + case (ViaConst(_), ViaVar(e)) => s"$e" + case (ViaConst(_), ViaConst(_)) => "" + } + val start = from match { + case ViaVar(_) => "s" + case ViaConst(s) => s"$s" + } + val end = to match { + case ViaVar(_) => "e" + case ViaConst(e) => s"$e" + } + val src = + s""" + | word output @0xc000 + | void main () { + | output = 0 + | loopy($args) + | } + | noinline void loopy($params) { + | $typ i + | for i,$start,$dir,$end { output += 1} + | } + """.stripMargin + EmuUnoptimizedCrossPlatformRun(platforms: _*)(src) { m => + m.readWord(0xc000) should equal(expectedCount) withClue s"(for i,$from,$dir,$to)" + } + } + def byteSubcase(from: IterationBound, dir: String, to: IterationBound, expectedCount: Int)(implicit pos: org.scalactic.source.Position, prettifier: Prettifier): Unit = { + subcase("byte", from, dir, to, expectedCount)(Cpu.Mos, Cpu.Z80, Cpu.Motorola6809) + } + def wordSubcase(from: IterationBound, dir: String, to: IterationBound, expectedCount: Int)(implicit pos: org.scalactic.source.Position, prettifier: Prettifier): Unit = { + subcase("word", from, dir, to, expectedCount)(Cpu.Mos, Cpu.Z80/*, Cpu.Motorola6809*/) + } + + test("Basic iteration count test") { + byteSubcase(ViaConst(1), "to", ViaConst(3), 3) + byteSubcase(ViaVar(1), "to", ViaConst(3), 3) + byteSubcase(ViaConst(1), "to", ViaVar(3), 3) + byteSubcase(ViaVar(1), "to", ViaVar(3), 3) + byteSubcase(ViaConst(1), "until", ViaConst(3), 2) + byteSubcase(ViaVar(1), "until", ViaConst(3), 2) + byteSubcase(ViaConst(1), "until", ViaVar(3), 2) + byteSubcase(ViaVar(1), "until", ViaVar(3), 2) + byteSubcase(ViaConst(11), "downto", ViaConst(3), 9) + byteSubcase(ViaVar(11), "downto", ViaConst(3), 9) + byteSubcase(ViaConst(11), "downto", ViaVar(3), 9) + byteSubcase(ViaVar(11), "downto", ViaVar(3), 9) + + wordSubcase(ViaConst(1), "to", ViaConst(3), 3) + wordSubcase(ViaVar(1), "to", ViaConst(3), 3) + wordSubcase(ViaConst(1), "to", ViaVar(3), 3) + wordSubcase(ViaVar(1), "to", ViaVar(3), 3) + wordSubcase(ViaConst(1), "until", ViaConst(3), 2) + wordSubcase(ViaVar(1), "until", ViaConst(3), 2) + wordSubcase(ViaConst(1), "until", ViaVar(3), 2) + wordSubcase(ViaVar(1), "until", ViaVar(3), 2) + wordSubcase(ViaConst(11), "downto", ViaConst(3), 9) + wordSubcase(ViaVar(11), "downto", ViaConst(3), 9) + wordSubcase(ViaConst(11), "downto", ViaVar(3), 9) + wordSubcase(ViaVar(11), "downto", ViaVar(3), 9) + } + + test("Edge case iteration count test") { + byteSubcase(ViaConst(0), "to", ViaConst(255), 256) + byteSubcase(ViaVar(0), "to", ViaConst(255), 256) + byteSubcase(ViaConst(0), "to", ViaVar(255), 256) + byteSubcase(ViaVar(0), "to", ViaVar(255), 256) + byteSubcase(ViaConst(0), "until", ViaConst(255), 255) + byteSubcase(ViaVar(0), "until", ViaConst(255), 255) + byteSubcase(ViaConst(0), "until", ViaVar(255), 255) + byteSubcase(ViaVar(0), "until", ViaVar(255), 255) + byteSubcase(ViaConst(255), "downto", ViaConst(0), 256) + byteSubcase(ViaVar(255), "downto", ViaConst(0), 256) + byteSubcase(ViaConst(255), "downto", ViaVar(0), 256) + byteSubcase(ViaVar(255), "downto", ViaVar(0), 256) + } + + test("Empty case iteration count test") { + byteSubcase(ViaConst(5), "until", ViaConst(5), 0) + byteSubcase(ViaVar(5), "until", ViaConst(5), 0) + byteSubcase(ViaConst(5), "until", ViaVar(5), 0) + byteSubcase(ViaVar(5), "until", ViaVar(5), 0) + + wordSubcase(ViaConst(5), "until", ViaConst(5), 0) + wordSubcase(ViaVar(5), "until", ViaConst(5), 0) + wordSubcase(ViaConst(5), "until", ViaVar(5), 0) + wordSubcase(ViaVar(5), "until", ViaVar(5), 0) + } + + test("Wrong directions in 'until' cases") { + byteSubcase(ViaConst(5), "until", ViaConst(4), 255) + byteSubcase(ViaVar(5), "until", ViaConst(4), 255) + byteSubcase(ViaConst(5), "until", ViaVar(4), 255) + byteSubcase(ViaVar(5), "until", ViaVar(4), 255) + + wordSubcase(ViaConst(65000), "until", ViaConst(0), 536) + wordSubcase(ViaVar(65000), "until", ViaConst(0), 536) + wordSubcase(ViaConst(65000), "until", ViaVar(0), 536) + wordSubcase(ViaVar(65000), "until", ViaVar(0), 536) + + wordSubcase(ViaConst(65000), "until", ViaConst(1), 537) + wordSubcase(ViaVar(65000), "until", ViaConst(1), 537) + wordSubcase(ViaConst(65000), "until", ViaVar(1), 537) + wordSubcase(ViaVar(65000), "until", ViaVar(1), 537) + } + + test("Wrong directions in 'to' cases") { + byteSubcase(ViaConst(1), "to", ViaConst(0), 256) + byteSubcase(ViaVar(1), "to", ViaConst(0), 256) + byteSubcase(ViaConst(1), "to", ViaVar(0), 256) + byteSubcase(ViaVar(1), "to", ViaVar(0), 256) + + wordSubcase(ViaConst(65000), "to", ViaConst(0), 537) + wordSubcase(ViaVar(65000), "to", ViaConst(0), 537) + wordSubcase(ViaConst(65000), "to", ViaVar(0), 537) + wordSubcase(ViaVar(65000), "to", ViaVar(0), 537) + } + + test("Wrong directions in 'downto' cases") { + byteSubcase(ViaConst(1), "downto", ViaConst(2), 256) + byteSubcase(ViaVar(1), "downto", ViaConst(2), 256) + byteSubcase(ViaConst(1), "downto", ViaVar(2), 256) + byteSubcase(ViaVar(1), "downto", ViaVar(2), 256) + + wordSubcase(ViaConst(1000), "downto", ViaConst(65000), 1537) + wordSubcase(ViaVar(1000), "downto", ViaConst(65000), 1537) + wordSubcase(ViaConst(1000), "downto", ViaVar(65000), 1537) + wordSubcase(ViaVar(1000), "downto", ViaVar(65000), 1537) + } +} diff --git a/src/test/scala/millfork/test/ForLoopSuite.scala b/src/test/scala/millfork/test/ForLoopSuite.scala index 61a8f64f..13ba46d1 100644 --- a/src/test/scala/millfork/test/ForLoopSuite.scala +++ b/src/test/scala/millfork/test/ForLoopSuite.scala @@ -166,15 +166,17 @@ class ForLoopSuite extends FunSuite with Matchers { | for i, zero, paralleluntil, ff { } | flag = 4 | for i, zero, parallelto, ff { } - | flag = 5 - | for i, ff, until, zero { _panic() } - | flag = 6 - | for i, ff, paralleluntil, zero { _panic() } - | flag = 7 - | for i, ff, paralleluntil, zero { _panic() } | flag = 8 | for i, zero, until, ff { } | flag = 9 + | for i, zero, until, zero { _panic() } + | flag = 10 + | for i, ff, until, ff { _panic() } + | flag = 11 + | for i, zero, paralleluntil, zero { _panic() } + | flag = 12 + | for i, ff, paralleluntil, ff { _panic() } + | flag = ff | } | void _panic(){while(true){}} """.stripMargin){ m=>