Fix incorrect equality for ArrayNew and friends, add missing rewrites in PrimitiveOps #104
Conversation
|
Made against |
|
I also have a refactoring of |
|
Alexey, concerning 554e4ad, the Manifest is already passed as an implicit parameter in Implicit version (creating array of integers, size 10): val arr = ArrayNew[Int](Const(10))Explicit version (creating array of integers, size 10): val arr = ArrayNew(Const(10))(manifest[Int])The implicits are in fact extra parameters in any method or class definition that they are part of. The Scala compiler attempts to match them automatically, and also provides the option to do this manually. The latter becomes a requirement in when dealing with several implicit methods or objects that makes the matching ambiguous. Have a look here to learn more: |
alexeyr
force-pushed
the
develop-0.9.x
branch
from
44dee0e
to
ee3a93e
Compare
|
@astojanov I am well aware of this. The issue I am concerned about, as mentioned in the commit message, is that implicit parameters aren't considered for equality of case classes. So if you already have an |
|
Or, now that I think of it, to check type as well as equality in |
alexeyr
force-pushed
the
develop-0.9.x
branch
from
ee3a93e
to
b1b2c08
Compare
|
Yes, it does work as expected in our case and removes a warning in existing tests. @TiarkRompf The last commit combines |
|
Consider the following code snippet: import org.scalatest.FunSpec
import scala.virtualization.lms.common._
class TestArrayEqual extends FunSpec {
val TestIR = new BaseFatExp with ArrayOpsExp
import TestIR._
def f[T:Manifest, U:Manifest] (): Rep[Unit] = {
val (t, u) = (NewArray[T](Const(17)), NewArray[U](Const(17)))
println("t = " + t)
println("u = " + u)
println("Comparing symbols: " + (t == u))
(t, u) match {
case (Def(Reflect(a@ArrayNew(_), _, _)), Def(Reflect(b@ArrayNew(_), _, _))) => {
println("Comparing case classes: " + (a == b && a.m == b.m))
}
}
val dt = findOrCreateDefinition(Reflect(ArrayNew[T](Const(17)), Alloc(), List()), t.pos)
val du = findOrCreateDefinition(Reflect(ArrayNew[U](Const(17)), Alloc(), List()), u.pos)
println("Comparing findOrCreateDefinition: " + (du == dt))
println()
}
describe("TestArray") {
reifyEffects(f[Int, Int]())
reifyEffects(f[Int, Double]())
}
}This will result with the following output: Obviously, in this case scenario
If a scenario exists where Its quite different scenario (which I am not considering here), if |
|
|
Well, ignoring import org.scalatest.FunSpec
import scala.virtualization.lms.common._
import scala.collection.immutable.{List => ScalaList}
class TestArrayEqual extends FunSpec {
val TestIR = new BaseFatExp with ArrayOpsExp with ListOpsExp with ImplicitOpsExp
import TestIR._
def f[T:Manifest, U:Manifest] (): Rep[Unit] = {
val (t, u) = (NewArray[T](Const(17)), NewArray[U](Const(17)))
println("t = " + t)
println("u = " + u)
println("Comparing symbols: " + (t == u))
(t, u) match {
case (Def(Reflect(a@ArrayNew(_), _, _)), Def(Reflect(b@ArrayNew(_), _, _))) => {
println("Comparing case classes: " + (a == b && a.m == b.m))
}
}
val dt = findOrCreateDefinition(Reflect(ArrayNew[T](Const(17)), Alloc(), ScalaList()), t.pos)
val du = findOrCreateDefinition(Reflect(ArrayNew[U](Const(17)), Alloc(), ScalaList()), u.pos)
println("dt: " + dt)
println("du: " + du)
println("Comparing findOrCreateDefinition: " + (du == dt))
println()
}
def arrayFromSeq[T:Manifest, U:Manifest] (sT: Seq[Exp[T]], sU: Seq[Exp[U]]): Rep[Unit] = {
val (t, u) = (array_obj_fromseq(sT), array_obj_fromseq(sU))
println("t = " + t)
println("u = " + u)
println("Comparing symbols: " + (t == u))
(t, u) match {
case (Def(a@ArrayFromSeq(_)), Def(b@ArrayFromSeq(_))) => {
println("Comparing case classes: " + (a == b && a.m == b.m))
}
}
val dt = findOrCreateDefinition(ArrayFromSeq(sT), t.pos)
val du = findOrCreateDefinition(ArrayFromSeq(sU), u.pos)
println("dt: " + dt)
println("du: " + du)
println("Comparing findOrCreateDefinition: " + (du == dt))
println()
}
def listNew[T:Manifest, U:Manifest] (sT: Seq[Exp[T]], sU: Seq[Exp[U]]): Rep[Unit] = {
val (t, u) = (list_new(sT), list_new(sU))
println("t = " + t)
println("u = " + u)
println("Comparing symbols: " + (t == u))
(t, u) match {
case (Def(a@ListNew(_)), Def(b@ListNew(_))) => {
println("Comparing case classes: " + (a == b))
}
}
val dt = findOrCreateDefinition(ListNew(sT), t.pos)
val du = findOrCreateDefinition(ListNew(sU), u.pos)
println("dt: " + dt)
println("du: " + du)
println("Comparing findOrCreateDefinition: " + (du == dt))
println()
}
def testImplicitOps[T:Manifest, U:Manifest] (sT: Exp[T], sU: Exp[U])
(implicit f1: T => String, f2: U => String): Rep[Unit] = {
val (t, u) = (implicit_convert[T, String](sT), implicit_convert[U, String](sU))
println("t = " + t)
println("u = " + u)
println("Comparing symbols: " + (t == u))
(t, u) match {
case (Def(a@ImplicitConvert(_)), Def(b@ImplicitConvert(_))) => {
println("Comparing case classes: " + (a == b))
}
}
val dt = findOrCreateDefinition(ImplicitConvert[T, String](sT), t.asInstanceOf[Exp[T]].pos)
val du = findOrCreateDefinition(ImplicitConvert[U, String](sU), u.asInstanceOf[Exp[U]].pos)
println("dt: " + dt)
println("du: " + du)
println("Comparing findOrCreateDefinition: " + (du == dt))
println()
}
describe("TestArray") {
println("Testing Mutable ArrayNew")
println("===========================================")
reifyEffects(f[Int, Int]())
reifyEffects(f[Int, Double]())
println("Testing Immutable ArrayFromSeq")
println("===========================================")
val seqA = Seq(fresh[Int], fresh[Int], fresh[Int])
val seqB = Seq(fresh[Int], fresh[Int], fresh[Int])
val seqC = Seq(fresh[Int], fresh[Int], fresh[Int])
val seqD = Seq(fresh[Double], fresh[Double], fresh[Double])
reifyEffects(arrayFromSeq(seqA, seqB))
reifyEffects(arrayFromSeq(seqC, seqD))
println("Testing Immutable ListNew")
println("===========================================")
val seqE = Seq(fresh[Int], fresh[Int], fresh[Int])
val seqF = Seq(fresh[Int], fresh[Int], fresh[Int])
val seqG = Seq(fresh[Int], fresh[Int], fresh[Int])
val seqH = Seq(fresh[Double], fresh[Double], fresh[Double])
reifyEffects(listNew(seqE, seqF))
reifyEffects(listNew(seqG, seqH))
println("Testing Immutable ImplicitOps")
println("===========================================")
val pInt = fresh[Int]
val qInt = fresh[Int]
val rInt = fresh[Int]
val sDbl = fresh[Double]
implicit val f1: (Int => String) = (t: Int ) => { t.toString }
implicit val f2: (Double => String) = (t: Double) => { t.toString }
reifyEffects(testImplicitOps(pInt, qInt))
reifyEffects(testImplicitOps(rInt, sDbl))
}
}and the obtained output is: as you can see, |
|
Also note that in 80fdb75, adding the manifest as an implicit argument in |
|
Consider instead: and |
It's passed to |
|
Yes, correct in the following case arrayFromSeq(Seq.empty[Exp[Int]], Seq.empty[Exp[Double]])you will obtain: which is expected in a way by the interface provided by LMS. Technically an empty array of println(Seq.empty[Exp[Int]] == Seq.empty[Exp[Double]])
println(Seq.empty[Int] == Seq.empty[Double])will result to: The type of the array will only matter if the array is in fact mutable, and you can not construct a mutable array using Therefore, I would say, if you really need to deal with behaviour that is different than the one that LMS is expecting, a better approach would be to override |
Not to the type system, they aren't! Arrays aren't covariant and you'll just get a type error if you try to use an empty The generated class will extend For a non-empty array example: |
|
I agree on this. Arrays are not covariant, and ideally if one wants to have an identical behaviour as the JVM, the representation of Arrays should deal with reifiable types. However, since staged version of arrays, namely I can not reproduce your latest example, but I do agree on the previous example concerning For the non-empty array example, this is exactly what happens, because it is expected. In Scala: scala> Seq[Double](1.0) == Seq[Int](1)
res0: Boolean = true |
Yes, it's expected currently. But again this gives you ill-typed generated code from well-typed LMS code as follows: (I've given the types explicitly, but that's how Scala would infer them as well). In generated code you should now get something like Though this pull request doesn't fully solve this case. It probably is necessary (and may be sufficient) to move the |
|
I finally see what you mean. And I agree, this needs to be fixed. I think the fix should not be limited only to Const(1) == Const(1.0)(which is, in a way, also an expected behaviour because |
|
Yes, it seems distinguishing them is necessary. I've also added an option for explicit types to help debug type errors in generated code (not a mixin trait to avoid problems with mixing it and |
alexeyr
force-pushed
the
develop-0.9.x
branch
2 times, most recently
from
42a29fd
to
0e21c85
Compare
alexeyr
force-pushed
the
develop-0.9.x
branch
from
0e21c85
to
f5df9fc
Compare
alexeyr
force-pushed
the
develop-0.9.x
branch
from
f5df9fc
to
25b2852
Compare
|
I finally had a chance to look at all the changes and discussion. Here is what I think:
|
|
Coming to think more about it, I wonder if the change to |
|
That's what I did originally, but the problem there is that 1) you need to also consider places where you have
Now you would need to be careful to add/remove the On the contrary, changes to |
That shouldn't be a problem in 1.0.x, because there is no implicit |
alexeyr
force-pushed
the
develop-0.9.x
branch
from
25b2852
to
afc2376
Compare
Moved to #110, though it will need to be updated after a decision on |
No description provided.