Skip to content

Fix #1167: Reduce the magic in Arrays.newRefArray. Implement multidimensional arrays #1188

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Apr 18, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion project/Build.scala
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ object DottyBuild extends Build {
com.typesafe.sbteclipse.plugin.EclipsePlugin.EclipseKeys.withSource := true,

// get libraries onboard
partestDeps := Seq("me.d-d" % "scala-compiler" % "2.11.5-20151022-113908-7fb0e653fd",
partestDeps := Seq("me.d-d" % "scala-compiler" % "2.11.5-20160322-171045-e19b30b3cd",
"org.scala-lang" % "scala-reflect" % scalaVersion.value,
"org.scala-lang" % "scala-library" % scalaVersion.value % "test"),
libraryDependencies ++= partestDeps.value,
Expand Down
35 changes: 5 additions & 30 deletions src/dotty/runtime/Arrays.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ package dotty.runtime

import scala.reflect.ClassTag

import java.lang.{reflect => jlr}

/** All but the first two operations should be short-circuited and implemented specially by
* the backend.
*/
Expand All @@ -22,35 +24,8 @@ object Arrays {
arr
}

/** Create an array of type T. T must be of form Array[E], with
* E being a reference type.
/** Create an array of a reference type T.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is obsolete.

*/
def newRefArray[T](length: Int): T = ???

/** Create a Byte[] array */
def newByteArray(length: Int): Array[Byte] = ???

/** Create a Short[] array */
def newShortArray(length: Int): Array[Short] = ???

/** Create a Char[] array */
def newCharArray(length: Int): Array[Char] = ???

/** Create an Int[] array */
def newIntArray(length: Int): Array[Int] = ???

/** Create a Long[] array */
def newLongArray(length: Int): Array[Long] = ???

/** Create a Float[] array */
def newFloatArray(length: Int): Array[Float] = ???

/** Create a Double[] array */
def newDoubleArray(length: Int): Array[Double] = ???

/** Create a Boolean[] array */
def newBooleanArray(length: Int): Array[Boolean] = ???

/** Create a scala.runtime.BoxedUnit[] array */
def newUnitArray(length: Int): Array[Unit] = ???
def newArray[Arr](componentType: Class[_], returnType: Class[Arr], dimensions: Array[Int]): Arr =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Am I right that the backend will eliminate these calls? If not it would be a potential performance problem, say if someone creates lots of 3-element Float vectors.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the intention!
If dimension.size ==1 backend will create a normal array
If dimension.size > 1 backend will issue multianewarray instruction.

See example here: https://github.com/dotty-staging/dotty/blob/remove-newarray-magic/tests/run/MultiArr.scala

jlr.Array.newInstance(componentType, dimensions: _*).asInstanceOf[Arr]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need returnType, here?

Also, why no bound? Right now componentType is completely unchecked; it might not be in sync. What about the following signature?

  def newArray[T](componentType: Class[T], returnType: Class[Arrary[T]], dimensions: Array[Int]): Array[T] =
    jlr.Array.newInstance(componentType, dimensions: _*).asInstanceOf[Arrary[T]]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need returnType, here?

I need to recreate the returned type at backend. And this is the easiest way I've found to do this.

Also, why no bound? Right now componentType is completely unchecked; it might not be in sync. What about the following signature?

It's NOT in sync before erasure. When you're creating a Array[Int], where Int is a primitive, you would like to pass https://docs.oracle.com/javase/7/docs/api/java/lang/Integer.html#TYPE, that is the Class for primitive Int, but is statically typed as Class[java.lang.Integer].

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that is the Class for primitive Int, but is statically typed as Class[java.lang.Integer].

Ouch! Why do we need this? Why can't we keep a Literal(Constant(defn.IntType))? That's how scalac does it, and I find it much more consistent.

}
9 changes: 1 addition & 8 deletions src/dotty/tools/backend/jvm/DottyBackendInterface.scala
Original file line number Diff line number Diff line change
Expand Up @@ -153,15 +153,8 @@ class DottyBackendInterface(outputDirectory: AbstractFile)(implicit ctx: Context
}.toMap
def unboxMethods: Map[Symbol, Symbol] = defn.ScalaValueClasses().map(x => (x, Erasure.Boxing.unboxMethod(x.asClass))).toMap

private val mkArrayNames: Set[Name] = Set("Byte", "Float", "Char", "Double", "Boolean", "Unit", "Long", "Int", "Short", "Ref").map{ x=>
("new" + x + "Array").toTermName
}

val dottyArraysModuleClass = toDenot(defn.DottyArraysModule).moduleClass.asClass


override def isSyntheticArrayConstructor(s: Symbol) = {
(toDenot(s).maybeOwner eq dottyArraysModuleClass) && mkArrayNames.contains(s.name)
s eq defn.newArrayMethod
}

def isBox(sym: Symbol): Boolean = Erasure.Boxing.isBox(sym)
Expand Down
44 changes: 21 additions & 23 deletions src/dotty/tools/backend/sjs/JSCodeGen.scala
Original file line number Diff line number Diff line change
Expand Up @@ -718,9 +718,9 @@ class JSCodeGen()(implicit ctx: Context) {
if (sym.is(Module)) {
assert(!sym.is(Package), "Cannot use package as value: " + tree)
genLoadModule(sym)
} else /*if (sym.isStaticMember) {
genStaticMember(sym)
} else if (paramAccessorLocals contains sym) {
} else if (sym.is(JavaStatic)) {
genLoadStaticField(sym)
} else /*if (paramAccessorLocals contains sym) {
paramAccessorLocals(sym).ref
} else if (isScalaJSDefinedJSClass(sym.owner)) {
val genQual = genExpr(qualifier)
Expand Down Expand Up @@ -1036,8 +1036,6 @@ class JSCodeGen()(implicit ctx: Context) {
genStringConcat(tree, receiver, args)
else if (code == HASH)
genScalaHash(tree, receiver)
else if (isArrayNew(code))
genArrayNew(tree, code)
else if (isArrayOp(code))
genArrayOp(tree, code)
else if (code == SYNCHRONIZED)
Expand Down Expand Up @@ -1409,24 +1407,6 @@ class JSCodeGen()(implicit ctx: Context) {
List(genExpr(receiver)))
}

/** Gen JS code for a new array operation. */
private def genArrayNew(tree: Tree, code: Int): js.Tree = {
import scala.tools.nsc.backend.ScalaPrimitives._

implicit val pos: Position = tree.pos

val Apply(fun, args) = tree
val genLength = genExpr(args.head)

toIRType(tree.tpe) match {
case arrayType: jstpe.ArrayType =>
js.NewArray(arrayType, List(genLength))

case irTpe =>
throw new FatalError(s"ArrayNew $tree must have an array type but was $irTpe")
}
}

/** Gen JS code for an array operation (get, set or length) */
private def genArrayOp(tree: Tree, code: Int): js.Tree = {
import scala.tools.nsc.backend.ScalaPrimitives._
Expand Down Expand Up @@ -2328,6 +2308,24 @@ class JSCodeGen()(implicit ctx: Context) {
}
}

/** Gen JS code for loading a Java static field.
*/
private def genLoadStaticField(sym: Symbol)(implicit pos: Position): js.Tree = {
/* Actually, there is no static member in Scala.js. If we come here, that
* is because we found the symbol in a Java-emitted .class in the
* classpath. But the corresponding implementation in Scala.js will
* actually be a val in the companion module.
*/

if (sym == defn.BoxedUnit_UNIT) {
js.Undefined()
} else {
val instance = genLoadModule(sym.owner)
val method = encodeStaticMemberSym(sym)
js.Apply(instance, method, Nil)(toIRType(sym.info))
}
}

/** Gen JS code for loading a module.
*
* Can be given either the module symbol, or its module class symbol.
Expand Down
12 changes: 0 additions & 12 deletions src/dotty/tools/backend/sjs/JSPrimitives.scala
Original file line number Diff line number Diff line change
Expand Up @@ -80,18 +80,6 @@ class JSPrimitives(ctx: Context) extends DottyPrimitives(ctx) {

val jsdefn = JSDefinitions.jsdefn

// For some reason, the JVM primitive set does not register those
addPrimitive(defn.DottyArraysModule.requiredMethod(Names.termName("newBooleanArray")), NEW_ZARRAY)
addPrimitive(defn.DottyArraysModule.requiredMethod(Names.termName("newByteArray")), NEW_BARRAY)
addPrimitive(defn.DottyArraysModule.requiredMethod(Names.termName("newShortArray")), NEW_SARRAY)
addPrimitive(defn.DottyArraysModule.requiredMethod(Names.termName("newCharArray")), NEW_CARRAY)
addPrimitive(defn.DottyArraysModule.requiredMethod(Names.termName("newIntArray")), NEW_IARRAY)
addPrimitive(defn.DottyArraysModule.requiredMethod(Names.termName("newLongArray")), NEW_LARRAY)
addPrimitive(defn.DottyArraysModule.requiredMethod(Names.termName("newFloatArray")), NEW_FARRAY)
addPrimitive(defn.DottyArraysModule.requiredMethod(Names.termName("newDoubleArray")), NEW_DARRAY)
addPrimitive(defn.DottyArraysModule.requiredMethod(Names.termName("newRefArray")), NEW_OARRAY)
addPrimitive(defn.DottyArraysModule.requiredMethod(Names.termName("newUnitArray")), NEW_OARRAY)

addPrimitive(defn.Any_getClass, GETCLASS)

for (i <- 0 to 22)
Expand Down
3 changes: 2 additions & 1 deletion src/dotty/tools/dotc/Compiler.scala
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,8 @@ class Compiler {
new Getters, // Replace non-private vals and vars with getter defs (fields are added later)
new ElimByName, // Expand by-name parameters and arguments
new AugmentScala2Traits, // Expand traits defined in Scala 2.11 to simulate old-style rewritings
new ResolveSuper), // Implement super accessors and add forwarders to trait methods
new ResolveSuper, // Implement super accessors and add forwarders to trait methods
new ArrayConstructors), // Intercept creation of (non-generic) arrays and intrinsify.
List(new Erasure), // Rewrite types to JVM model, erasing all type parameters, abstract types and refinements.
List(new ElimErasedValueType, // Expand erased value types to their underlying implmementation types
new VCElideAllocations, // Peep-hole optimization to eliminate unnecessary value class allocations
Expand Down
31 changes: 16 additions & 15 deletions src/dotty/tools/dotc/ast/tpd.scala
Original file line number Diff line number Diff line change
Expand Up @@ -125,8 +125,8 @@ object tpd extends Trees.Instance[Type] with TypedTreeInfo {
def SeqLiteral(elems: List[Tree], elemtpt: Tree)(implicit ctx: Context): SeqLiteral =
ta.assignType(untpd.SeqLiteral(elems, elemtpt), elems, elemtpt)

def JavaSeqLiteral(elems: List[Tree], elemtpt: Tree)(implicit ctx: Context): SeqLiteral =
ta.assignType(new untpd.JavaSeqLiteral(elems, elemtpt), elems, elemtpt)
def JavaSeqLiteral(elems: List[Tree], elemtpt: Tree)(implicit ctx: Context): JavaSeqLiteral =
ta.assignType(new untpd.JavaSeqLiteral(elems, elemtpt), elems, elemtpt).asInstanceOf[JavaSeqLiteral]

def TypeTree(original: Tree)(implicit ctx: Context): TypeTree =
TypeTree(original.tpe, original)
Expand Down Expand Up @@ -362,18 +362,16 @@ object tpd extends Trees.Instance[Type] with TypedTreeInfo {
* kind for the given element type in `typeArg`. No type arguments or
* `length` arguments are given.
*/
def newArray(typeArg: Tree, pos: Position)(implicit ctx: Context): Tree = {
val elemType = typeArg.tpe
val elemClass = elemType.classSymbol
def newArr(kind: String) =
ref(defn.DottyArraysModule).select(s"new${kind}Array".toTermName).withPos(pos)
if (TypeErasure.isUnboundedGeneric(elemType))
newArr("Generic").appliedToTypeTrees(typeArg :: Nil)
else if (elemClass.isPrimitiveValueClass)
newArr(elemClass.name.toString)
else
newArr("Ref").appliedToTypeTrees(
TypeTree(defn.ArrayOf(elemType)).withPos(typeArg.pos) :: Nil)
def newArray(elemTpe: Type, returnTpe: Type, pos: Position, dims: JavaSeqLiteral)(implicit ctx: Context): Tree = {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum, the signature of this method changes, but I see no call site change in this commit. That's suspicious ;)

val elemClass = elemTpe.classSymbol
def newArr =
ref(defn.DottyArraysModule).select(defn.newArrayMethod).withPos(pos)

if (!ctx.erasedTypes) {
assert(!TypeErasure.isUnboundedGeneric(elemTpe)) //needs to be done during typer. See Applications.convertNewGenericArray
newArr.appliedToTypeTrees(TypeTree(returnTpe) :: Nil).appliedToArgs(clsOf(elemTpe) :: clsOf(returnTpe) :: dims :: Nil).withPos(pos)
} else // after erasure
newArr.appliedToArgs(clsOf(elemTpe) :: clsOf(returnTpe) :: dims :: Nil).withPos(pos)
}

// ------ Creating typed equivalents of trees that exist only in untyped form -------
Expand Down Expand Up @@ -835,7 +833,10 @@ object tpd extends Trees.Instance[Type] with TypedTreeInfo {
case tpnme.Float => TYPE(defn.BoxedFloatModule)
case tpnme.Double => TYPE(defn.BoxedDoubleModule)
case tpnme.Unit => TYPE(defn.BoxedUnitModule)
case _ => Literal(Constant(TypeErasure.erasure(tp)))
case _ =>
if(ctx.erasedTypes || !tp.derivesFrom(defn.ArrayClass))
Literal(Constant(TypeErasure.erasure(tp)))
else Literal(Constant(tp))
}
}

Expand Down
9 changes: 6 additions & 3 deletions src/dotty/tools/dotc/core/Definitions.scala
Original file line number Diff line number Diff line change
Expand Up @@ -246,8 +246,8 @@ class Definitions {
def DottyPredefModule(implicit ctx: Context) = DottyPredefModuleRef.symbol
lazy val DottyArraysModuleRef = ctx.requiredModuleRef("dotty.runtime.Arrays")
def DottyArraysModule(implicit ctx: Context) = DottyArraysModuleRef.symbol

def newRefArrayMethod(implicit ctx: Context) = DottyArraysModule.requiredMethod("newRefArray")
def newGenericArrayMethod(implicit ctx: Context) = DottyArraysModule.requiredMethod("newGenericArray")
def newArrayMethod(implicit ctx: Context) = DottyArraysModule.requiredMethod("newArray")

lazy val NilModuleRef = ctx.requiredModuleRef("scala.collection.immutable.Nil")
def NilModule(implicit ctx: Context) = NilModuleRef.symbol
Expand Down Expand Up @@ -279,6 +279,9 @@ class Definitions {
def Array_clone(implicit ctx: Context) = Array_cloneR.symbol
lazy val ArrayConstructorR = ArrayClass.requiredMethodRef(nme.CONSTRUCTOR)
def ArrayConstructor(implicit ctx: Context) = ArrayConstructorR.symbol
lazy val ArrayModuleType = ctx.requiredModuleRef("scala.Array")
def ArrayModule(implicit ctx: Context) = ArrayModuleType.symbol.moduleClass.asClass


lazy val UnitType: TypeRef = valueTypeRef("scala.Unit", BoxedUnitType, java.lang.Void.TYPE, UnitEnc)
def UnitClass(implicit ctx: Context) = UnitType.symbol.asClass
Expand Down Expand Up @@ -622,7 +625,7 @@ class Definitions {
lazy val PhantomClasses = Set[Symbol](AnyClass, AnyValClass, NullClass, NothingClass)

def isPolymorphicAfterErasure(sym: Symbol) =
(sym eq Any_isInstanceOf) || (sym eq Any_asInstanceOf) || (sym eq newRefArrayMethod)
(sym eq Any_isInstanceOf) || (sym eq Any_asInstanceOf)

def isTupleType(tp: Type)(implicit ctx: Context) = {
val arity = tp.dealias.argInfos.length
Expand Down
57 changes: 57 additions & 0 deletions src/dotty/tools/dotc/transform/ArrayConstructors.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
package dotty.tools.dotc
package transform

import core._
import TreeTransforms._
import Contexts.Context
import Flags._
import SymUtils._
import Symbols._
import SymDenotations._
import Types._
import Decorators._
import DenotTransformers._
import StdNames._
import NameOps._
import ast.Trees._
import dotty.tools.dotc.ast.tpd
import util.Positions._
import Names._

import collection.mutable
import ResolveSuper._

import scala.collection.immutable.::


/** This phase rewrites calls to array constructors to newArray method in Dotty.runtime.Arrays module.
*
* It assummes that generic arrays have already been handled by typer(see Applications.convertNewGenericArray).
* Additionally it optimizes calls to scala.Array.ofDim functions by replacing them with calls to newArray with specific dimensions
*/
class ArrayConstructors extends MiniPhaseTransform { thisTransform =>
import ast.tpd._

override def phaseName: String = "arrayConstructors"

override def transformApply(tree: tpd.Apply)(implicit ctx: Context, info: TransformerInfo): tpd.Tree = {
def rewrite(elemType: Type, dims: List[Tree]) =
tpd.newArray(elemType, tree.tpe, tree.pos, JavaSeqLiteral(dims, TypeTree(defn.IntClass.typeRef)))

if (tree.fun.symbol eq defn.ArrayConstructor) {
val TypeApply(tycon, targ :: Nil) = tree.fun
rewrite(targ.tpe, tree.args)
} else if ((tree.fun.symbol.maybeOwner eq defn.ArrayModule) && (tree.fun.symbol.name eq nme.ofDim) && !tree.tpe.isInstanceOf[MethodicType]) {

tree.fun match {
case Apply(TypeApply(t: Ident, targ), dims) if !TypeErasure.isUnboundedGeneric(targ.head.tpe) =>
rewrite(targ.head.tpe, dims)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens to the implicit ClassTag argument to ofDim? Isn't part of dims? In that case, rewrite would do something bad with it wouldn't it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

implicit ClassTag argument to ofDim is the argument of tree: Apply, that has a form
Apply(Apply(TypeApply(_, targs), dims, classTag).
I'm deconstructing the inner Apply here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right. OK.

case Apply(TypeApply(t: Select, targ), dims) if !TypeErasure.isUnboundedGeneric(targ.head.tpe) =>
Block(t.qualifier :: Nil, rewrite(targ.head.tpe, dims))
case _ => tree
}

} else tree
}
}

23 changes: 7 additions & 16 deletions src/dotty/tools/dotc/transform/Erasure.scala
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import core.Decorators._
import dotty.tools.dotc.ast.{Trees, tpd, untpd}
import ast.Trees._
import scala.collection.mutable.ListBuffer
import dotty.tools.dotc.core.Flags
import dotty.tools.dotc.core.{Constants, Flags}
import ValueClasses._
import TypeUtils._
import ExplicitOuter._
Expand Down Expand Up @@ -299,8 +299,9 @@ object Erasure extends TypeTestsCasts{
assignType(untpd.cpy.Typed(tree)(expr1, tpt1), tpt1)
}

override def typedLiteral(tree: untpd.Literal)(implicit ctc: Context): Literal =
override def typedLiteral(tree: untpd.Literal)(implicit ctx: Context): Literal =
if (tree.typeOpt.isRef(defn.UnitClass)) tree.withType(tree.typeOpt)
else if (tree.const.tag == Constants.ClazzTag) Literal(Constant(erasure(tree.const.typeValue)))
else super.typedLiteral(tree)

/** Type check select nodes, applying the following rewritings exhaustively
Expand Down Expand Up @@ -467,28 +468,18 @@ object Erasure extends TypeTestsCasts{
tpt = untpd.TypedSplice(TypeTree(sym.info).withPos(vdef.tpt.pos))), sym)

override def typedDefDef(ddef: untpd.DefDef, sym: Symbol)(implicit ctx: Context) = {
var effectiveSym = sym
if (sym == defn.newRefArrayMethod) {
// newRefArray is treated specially: It's the only source-defined method
// that has a polymorphic type after erasure. But treating its (dummy) definition
// with a polymorphic type at and after erasure is an awkward special case.
// We therefore rewrite the method definition with a new Symbol of type
// (length: Int)Object
val MethodType(pnames, ptypes) = sym.info.resultType
effectiveSym = sym.copy(info = MethodType(pnames, ptypes, defn.ObjectType))
}
val restpe =
if (effectiveSym.isConstructor) defn.UnitType
else effectiveSym.info.resultType
if (sym.isConstructor) defn.UnitType
else sym.info.resultType
val ddef1 = untpd.cpy.DefDef(ddef)(
tparams = Nil,
vparamss = (outer.paramDefs(effectiveSym) ::: ddef.vparamss.flatten) :: Nil,
vparamss = (outer.paramDefs(sym) ::: ddef.vparamss.flatten) :: Nil,
tpt = untpd.TypedSplice(TypeTree(restpe).withPos(ddef.tpt.pos)),
rhs = ddef.rhs match {
case id @ Ident(nme.WILDCARD) => untpd.TypedSplice(id.withType(restpe))
case _ => ddef.rhs
})
super.typedDefDef(ddef1, effectiveSym)
super.typedDefDef(ddef1, sym)
}

/** After erasure, we may have to replace the closure method by a bridge.
Expand Down
3 changes: 1 addition & 2 deletions src/dotty/tools/dotc/transform/TreeChecker.scala
Original file line number Diff line number Diff line change
Expand Up @@ -359,8 +359,7 @@ class TreeChecker extends Phase with SymTransformer {
def isNonMagicalMethod(x: Symbol) =
x.is(Method) &&
!x.isCompanionMethod &&
!x.isValueClassConvertMethod &&
x != defn.newRefArrayMethod
!x.isValueClassConvertMethod

val symbolsNotDefined = cls.classInfo.decls.toSet.filter(isNonMagicalMethod) -- impl.body.map(_.symbol) - constr.symbol

Expand Down
Loading