Merge pull request scala#4602 from lrytz/yoptInScala

Fix optimizer bugs and enable -Yopt:l:classpath when building scala

Author: adriaanm

build-ant-macros.xml

@@ -6,7 +6,7 @@
     <attribute name="name"/>
     <sequential>
       <antcall target="@{name}">
-        <param name="scalac.args.optimise" value="-optimise"/>
+        <param name="scalac.args.optimise" value="-Yopt:l:classpath"/>
       </antcall>
     </sequential>
   </macrodef>

build.xml

@@ -19,7 +19,7 @@ ant $antArgs $scalacArgs $targets
 
 antArgs tend to be:
  -Darchives.skipxz=true
- -Dscalac.args.optimise=-optimise
+ -Dscalac.args.optimise=-Yopt:l:classpath
 
 scalacArgs examples:
   "-Dscalac.args=\"-Yrangepos\" -Dpartest.scalac_opts=\"-Yrangepos\""
@@ -79,13 +79,13 @@ TODO:
   <target name="publish-opt-nodocs" description="Publishes Scala (optimized) without generating docs/testing (library/reflect/compiler/swing).">
     <antcall target="publish">
       <param name="docs.skip" value="1"/>
-      <param name="scalac.args.optimise" value="-optimise"/>
+      <param name="scalac.args.optimise" value="-Yopt:l:classpath"/>
     </antcall>
   </target>
   <target name="publish-core-opt-nodocs" description="Builds an untested, undocumented optimised core (library/reflect/compiler) and publishes to maven.">
     <antcall target="publish-core">
       <param name="docs.skip" value="1"/>
-      <param name="scalac.args.optimise" value="-optimise"/>
+      <param name="scalac.args.optimise" value="-Yopt:l:classpath"/>
     </antcall>
   </target>
   <target name="publish-core-local-nodocs" description="Builds an untested, undocumented core (library/reflect/compiler) and locally publishes to maven">
@@ -1042,7 +1042,7 @@ TODO:
     <antcall target="publish-core-local">
       <param name="maven.version.suffix" value="-STARR-${git.commit.sha}-SNAPSHOT"/>
       <param name="docs.skip" value="1"/>
-      <param name="scalac.args.optimise" value="-optimise"/>
+      <param name="scalac.args.optimise" value="-Yopt:l:classpath"/>
       <param name="update.starr.version" value="alright then"/>
     </antcall>
   </target>

scripts/jobs/integrate/bootstrap

@@ -493,7 +493,7 @@ bootstrap() {
       -Dremote.snapshot.repository=NOPE\
       -Dremote.release.repository=$releaseTempRepoUrl\
       -Drepository.credentials.id=$releaseTempRepoCred\
-      -Dscalac.args.optimise=-optimise\
+      -Dscalac.args.optimise=-Yopt:l:classpath\
       -Ddocs.skip=1\
       -Dlocker.skip=1\
       $publishLockerPrivateTask >> $baseDir/logs/builds 2>&1
@@ -533,7 +533,7 @@ bootstrap() {
       -Dremote.snapshot.repository=NOPE\
       -Dremote.release.repository=$releaseTempRepoUrl\
       -Drepository.credentials.id=$releaseTempRepoCred\
-      -Dscalac.args.optimise=-optimise\
+      -Dscalac.args.optimise=-Yopt:l:classpath\
       $antBuildTask $publishPrivateTask
 
   # clear ivy cache (and to be sure, local as well), so the next round of sbt builds sees the fresh scala

scripts/jobs/validate/test

@@ -10,7 +10,7 @@ case $prDryRun in
     # build quick using STARR built upstream, as specified by scalaVersion
     # (in that sense it's locker, since it was built with starr by that upstream job)
     ant -Dstarr.version=$scalaVersion \
-       -Dscalac.args.optimise=-optimise \
+       -Dscalac.args.optimise=-Yopt:l:classpath \
        -Dlocker.skip=1 -Dextra.repo.url=$prRepoUrl \
        $testExtraArgs ${testTarget-test.core docs.done}
     ;;

src/compiler/scala/tools/nsc/Global.scala

@@ -1373,7 +1373,7 @@ class Global(var currentSettings: Settings, var reporter: Reporter)
       unitbuf += unit
       compiledFiles += unit.source.file.path
     }
-    private def checkDeprecatedSettings(unit: CompilationUnit) {
+    private def warnDeprecatedAndConflictingSettings(unit: CompilationUnit) {
       // issue warnings for any usage of deprecated settings
       settings.userSetSettings filter (_.isDeprecated) foreach { s =>
         currentRun.reporting.deprecationWarning(NoPosition, s.name + " is deprecated: " + s.deprecationMessage.get)
@@ -1383,6 +1383,7 @@ class Global(var currentSettings: Settings, var reporter: Reporter)
         currentRun.reporting.deprecationWarning(NoPosition, settings.target.name + ":" + settings.target.value + " is deprecated and has no effect, setting to " + supportedTarget)
         settings.target.value = supportedTarget
       }
+      settings.conflictWarning.foreach(reporter.warning(NoPosition, _))
     }
 
     /* An iterator returning all the units being compiled in this run */
@@ -1473,7 +1474,7 @@ class Global(var currentSettings: Settings, var reporter: Reporter)
     def compileSources(sources: List[SourceFile]) = if (!reporter.hasErrors) {
 
       def checkDeprecations() = {
-        checkDeprecatedSettings(newCompilationUnit(""))
+        warnDeprecatedAndConflictingSettings(newCompilationUnit(""))
         reporting.summarizeErrors()
       }
 
@@ -1495,7 +1496,7 @@ class Global(var currentSettings: Settings, var reporter: Reporter)
       val startTime = currentTime
 
       reporter.reset()
-      checkDeprecatedSettings(unitbuf.head)
+      warnDeprecatedAndConflictingSettings(unitbuf.head)
       globalPhase = fromPhase
 
       while (globalPhase.hasNext && !reporter.hasErrors) {

src/compiler/scala/tools/nsc/backend/jvm/AsmUtils.scala

@@ -10,6 +10,7 @@ import java.io.{StringWriter, PrintWriter}
 import scala.tools.asm.util.{CheckClassAdapter, TraceClassVisitor, TraceMethodVisitor, Textifier}
 import scala.tools.asm.{ClassWriter, Attribute, ClassReader}
 import scala.collection.convert.decorateAsScala._
+import scala.tools.nsc.backend.jvm.analysis.InitialProducer
 import scala.tools.nsc.backend.jvm.opt.InlineInfoAttributePrototype
 
 object AsmUtils {
@@ -81,13 +82,16 @@ object AsmUtils {
   /**
    * Returns a human-readable representation of the given instruction.
    */
-  def textify(insn: AbstractInsnNode): String = {
-    val trace = new TraceMethodVisitor(new Textifier)
-    insn.accept(trace)
-    val sw = new StringWriter
-    val pw = new PrintWriter(sw)
-    trace.p.print(pw)
-    sw.toString.trim
+  def textify(insn: AbstractInsnNode): String = insn match {
+    case _: InitialProducer =>
+      insn.toString
+    case _ =>
+      val trace = new TraceMethodVisitor(new Textifier)
+      insn.accept(trace)
+      val sw = new StringWriter
+      val pw = new PrintWriter(sw)
+      trace.p.print(pw)
+      sw.toString.trim
   }
 
   /**

src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala

@@ -843,7 +843,6 @@ abstract class BCodeBodyBuilder extends BCodeSkelBuilder {
          *
          * New (http://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.10.1)
          *   - Requires consistent stack map frames. GenBCode always generates stack frames.
-         *     or higher.
          *   - In practice: the ASM library computes stack map frames for us (ClassWriter). Emitting
          *     correct frames after an ATHROW is probably complex, so ASM uses the following strategy:
          *       - Every time when generating an ATHROW, a new basic block is started.

src/compiler/scala/tools/nsc/backend/jvm/BCodeHelpers.scala

@@ -728,11 +728,11 @@ abstract class BCodeHelpers extends BCodeIdiomatic with BytecodeWriters {
         mv.visitVarInsn(ALOAD, 1)
         mv.visitFieldInsn(PUTSTATIC, clazz.javaBinaryName.toString, "$deserializeLambdaCache$", "Ljava/util/Map;")
         mv.visitLabel(l0)
-        mv.visitFrame(asm.Opcodes.F_APPEND,1, Array("java/util/Map"), 0, null)
+        mv.visitFieldInsn(GETSTATIC, "scala/runtime/LambdaDeserializer$", "MODULE$", "Lscala/runtime/LambdaDeserializer$;")
         mv.visitMethodInsn(INVOKESTATIC, "java/lang/invoke/MethodHandles", "lookup", "()Ljava/lang/invoke/MethodHandles$Lookup;", false)
         mv.visitVarInsn(ALOAD, 1)
         mv.visitVarInsn(ALOAD, 0)
-        mv.visitMethodInsn(INVOKESTATIC, "scala/runtime/LambdaDeserializer", "deserializeLambda", "(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/util/Map;Ljava/lang/invoke/SerializedLambda;)Ljava/lang/Object;", false)
+        mv.visitMethodInsn(INVOKEVIRTUAL, "scala/runtime/LambdaDeserializer$", "deserializeLambda", "(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/util/Map;Ljava/lang/invoke/SerializedLambda;)Ljava/lang/Object;", false)
         mv.visitInsn(ARETURN)
         mv.visitEnd()
       }

src/compiler/scala/tools/nsc/backend/jvm/analysis/ProdConsAnalyzer.scala

@@ -103,7 +103,11 @@ class ProdConsAnalyzer(methodNode: MethodNode, classInternalName: InternalName)
   def initialProducersForValueAt(insn: AbstractInsnNode, slot: Int): Set[AbstractInsnNode] = {
     def initialProducers(insn: AbstractInsnNode, producedSlot: Int): Set[AbstractInsnNode] = {
       if (isCopyOperation(insn)) {
-        _initialProducersCache.getOrElseUpdate((insn, producedSlot), {
+        val key = (insn, producedSlot)
+        _initialProducersCache.getOrElseUpdate(key, {
+          // prevent infinite recursion if an instruction is its own producer or consumer
+          // see cyclicProdCons in ProdConsAnalyzerTest
+          _initialProducersCache(key) = Set.empty
           val (sourceValue, sourceValueSlot) = copyOperationSourceValue(insn, producedSlot)
           sourceValue.insns.iterator.asScala.flatMap(initialProducers(_, sourceValueSlot)).toSet
         })
@@ -121,7 +125,11 @@ class ProdConsAnalyzer(methodNode: MethodNode, classInternalName: InternalName)
   def ultimateConsumersOfValueAt(insn: AbstractInsnNode, slot: Int): Set[AbstractInsnNode] = {
     def ultimateConsumers(insn: AbstractInsnNode, consumedSlot: Int): Set[AbstractInsnNode] = {
       if (isCopyOperation(insn)) {
-        _ultimateConsumersCache.getOrElseUpdate((insn, consumedSlot), {
+        val key = (insn, consumedSlot)
+        _ultimateConsumersCache.getOrElseUpdate(key, {
+          // prevent infinite recursion if an instruction is its own producer or consumer
+          // see cyclicProdCons in ProdConsAnalyzerTest
+          _ultimateConsumersCache(key) = Set.empty
           for {
             producedSlot     <- copyOperationProducedValueSlots(insn, consumedSlot)
             consumer         <- consumersOfValueAt(insn.getNext, producedSlot)

src/compiler/scala/tools/nsc/backend/jvm/opt/BytecodeUtils.scala

@@ -12,7 +12,7 @@ import scala.collection.mutable
 import scala.reflect.internal.util.Collections._
 import scala.tools.asm.commons.CodeSizeEvaluator
 import scala.tools.asm.tree.analysis._
-import scala.tools.asm.{MethodWriter, ClassWriter, Label, Opcodes}
+import scala.tools.asm.{MethodWriter, ClassWriter, Label, Opcodes, Type}
 import scala.tools.asm.tree._
 import GenBCode._
 import scala.collection.convert.decorateAsScala._
@@ -330,6 +330,26 @@ object BytecodeUtils {
     )).toList
   }
 
+  /**
+   * This method is used by optimizer components to eliminate phantom values of instruction
+   * that load a value of type `Nothing$` or `Null$`. Such values on the stack don't interact well
+   * with stack map frames.
+   *
+   * For example, `opt.getOrElse(throw e)` is re-written to an invocation of the lambda body, a
+   * method with return type `Nothing$`. Similarly for `opt.getOrElse(null)` and `Null$`.
+   *
+   * During bytecode generation this is handled by BCodeBodyBuilder.adapt. See the comment in that
+   * method which explains the issue with such phantom values.
+   */
+  def fixLoadedNothingOrNullValue(loadedType: Type, loadInstr: AbstractInsnNode, methodNode: MethodNode, bTypes: BTypes): Unit = {
+    if (loadedType == bTypes.coreBTypes.RT_NOTHING.toASMType) {
+      methodNode.instructions.insert(loadInstr, new InsnNode(Opcodes.ATHROW))
+    } else if (loadedType == bTypes.coreBTypes.RT_NULL.toASMType) {
+      methodNode.instructions.insert(loadInstr, new InsnNode(Opcodes.ACONST_NULL))
+      methodNode.instructions.insert(loadInstr, new InsnNode(Opcodes.POP))
+    }
+  }
+
   /**
    * A wrapper to make ASM's Analyzer a bit easier to use.
    */

src/compiler/scala/tools/nsc/backend/jvm/opt/ClosureOptimizer.scala

@@ -283,6 +283,10 @@ class ClosureOptimizer[BT <: BTypes](val btypes: BT) {
           val isInterface = bodyOpcode == INVOKEINTERFACE
           val bodyInvocation = new MethodInsnNode(bodyOpcode, lambdaBodyHandle.getOwner, lambdaBodyHandle.getName, lambdaBodyHandle.getDesc, isInterface)
           methodNode.instructions.insertBefore(invocation, bodyInvocation)
+
+          val returnType = Type.getReturnType(lambdaBodyHandle.getDesc)
+          fixLoadedNothingOrNullValue(returnType, bodyInvocation, methodNode, btypes) // see comment of that method
+
           methodNode.instructions.remove(invocation)
 
           // update the call graph

src/compiler/scala/tools/nsc/settings/ScalaSettings.scala

@@ -136,7 +136,7 @@ trait ScalaSettings extends AbsScalaSettings
   val sourceReader       = StringSetting       ("-Xsource-reader", "classname", "Specify a custom method for reading source files.", "")
   val reporter           = StringSetting       ("-Xreporter", "classname", "Specify a custom reporter for compiler messages.", "scala.tools.nsc.reporters.ConsoleReporter")
   val strictInference    = BooleanSetting      ("-Xstrict-inference", "Don't infer known-unsound types")
-  val source             = ScalaVersionSetting ("-Xsource", "version", "Treat compiler input as Scala source for the specified version, see SI-8126.", initial = ScalaVersion("2.11"))
+  val source             = ScalaVersionSetting ("-Xsource", "version", "Treat compiler input as Scala source for the specified version, see SI-8126.", initial = ScalaVersion("2.12"))
 
   val XnoPatmatAnalysis = BooleanSetting ("-Xno-patmat-analysis", "Don't perform exhaustivity/unreachability analysis. Also, ignore @switch annotation.")
   val XfullLubs         = BooleanSetting ("-Xfull-lubs", "Retains pre 2.10 behavior of less aggressive truncation of least upper bounds.")
@@ -388,6 +388,23 @@ trait ScalaSettings extends AbsScalaSettings
     val Normal = "normal"
     val Discard = "discard"
   }
+
+  def conflictWarning: Option[String] = {
+    def oldOptimiseFlagsInGenBCode: Option[String] = {
+      val optFlags: List[Setting] = if (optimise.value) List(optimise) else optimiseSettings.filter(_.value)
+      if (isBCodeActive && optFlags.nonEmpty) {
+        val msg = s"""Compiler settings for the 2.11 optimizer (${optFlags.map(_.name).mkString(", ")}) are incompatible with -Ybackend:GenBCode (which is the default in 2.12).
+                     |The optimizer settings are ignored. See -Yopt:help for enabling the new optimizer in 2.12.""".stripMargin
+        Some(msg)
+      } else
+        None
+    }
+
+    List(oldOptimiseFlagsInGenBCode /*, moreToCome */).flatten match {
+      case Nil => None
+      case warnings => Some("Conflicting compiler settings were detected. Some settings will be ignored.\n" + warnings.mkString("\n"))
+    }
+  }
 }
 
 object ClassPathRepresentationType {

test/files/neg/incompatibleSettings.check

@@ -0,0 +1,6 @@
+warning: Conflicting compiler settings were detected. Some settings will be ignored.
+Compiler settings for the 2.11 optimizer (-optimise) are incompatible with -Ybackend:GenBCode (which is the default in 2.12).
+The optimizer settings are ignored. See -Yopt:help for enabling the new optimizer in 2.12.
+error: No warnings can be incurred under -Xfatal-warnings.
+one warning found
+one error found

test/files/neg/incompatibleSettings.flags

@@ -0,0 +1 @@
+-optimise -Ybackend:GenBCode -Xfatal-warnings

test/files/neg/incompatibleSettings.scala

@@ -0,0 +1 @@
+class C

test/files/neg/sealed-final-neg.flags

@@ -1 +1 @@
--Xfatal-warnings -Yinline-warnings -optimise
+-Xfatal-warnings -Ybackend:GenASM -Yinline-warnings -optimise

test/files/neg/t4425.flags

@@ -0,0 +1 @@
+-Xsource:2.11

test/files/neg/t5639b.flags

@@ -1 +1 @@
--optimise -Xfatal-warnings -Yinline-warnings
+-optimise -Ybackend:GenASM -Xfatal-warnings -Yinline-warnings

test/files/pos/inline-access-levels.flags

@@ -1 +1 @@
--optimise -Xfatal-warnings
+-optimise -Ybackend:GenASM -Xfatal-warnings

test/files/pos/inliner2.flags

@@ -1 +1 @@
--Xfatal-warnings -Yinline-warnings -optimise
+-Xfatal-warnings -Yinline-warnings -Ybackend:GenASM -optimise

test/files/pos/sealed-final.flags

@@ -1 +1 @@
--optimise -Xfatal-warnings
+-optimise -Ybackend:GenASM -Xfatal-warnings

test/files/pos/t3420.flags

@@ -1 +1 @@
--optimise -Xfatal-warnings -deprecation:false -Yinline-warnings:false
+-optimise -Ybackend:GenASM -Xfatal-warnings -deprecation:false -Yinline-warnings:false

test/files/pos/t8410.flags

@@ -1,3 +1,6 @@
+/*
+ * filter: inliner warnings; re-run with
+ */
 import scala.tools.nsc.interactive.tests._
 import scala.reflect.internal.util._
 

test/files/presentation/t7678/Runner.scala

@@ -16,7 +16,7 @@ import Opcodes._
 // verify. So the test includes a version check that short-circuites the whole test
 // on JDK 6
 object Test extends DirectTest {
-  override def extraSettings: String = "-optimise -usejavacp -d " + testOutput.path + " -cp " + testOutput.path
+  override def extraSettings: String = "-Yopt:l:classpath -usejavacp -d " + testOutput.path + " -cp " + testOutput.path
 
   def generateClass() {
     val invokerClassName =  "DynamicInvoker"

test/files/run/blame_eye_triple_eee-double.flags

@@ -13,7 +13,7 @@ import Opcodes._
 // By its nature the test can only work on JDK 8+ because under JDK 7- the
 // interface won't verify.
 object Test extends DirectTest {
-  override def extraSettings: String = "-optimise -usejavacp -d " + testOutput.path + " -cp " + testOutput.path
+  override def extraSettings: String = "-Yopt:l:classpath -usejavacp -d " + testOutput.path + " -cp " + testOutput.path
 
   def generateInterface() {
     val interfaceName =  "HasDefaultMethod"

test/files/run/blame_eye_triple_eee-float.flags

@@ -1 +1 @@
--optimise
+-optimise -Ybackend:GenASM

test/files/run/classfile-format-51.scala

@@ -1 +1 @@
--optimize
+-optimize -Ybackend:GenASM

test/files/run/classfile-format-52.scala

@@ -1 +1 @@
--optimise -Xelide-below 900
+-optimise -Ybackend:GenASM -Xelide-below 900

test/files/run/constant-optimization.flags

@@ -1,3 +1,6 @@
+/*
+ * filter: inliner warnings; re-run with
+ */
 import annotation._
 import elidable._
 

test/files/run/dead-code-elimination.flags

@@ -1 +1 @@
--Yoverride-vars -Yinline
+-Yoverride-vars -Yinline -Ybackend:GenASM

test/files/run/elidable-opt.flags

@@ -36,7 +36,7 @@ object Test extends DirectTest {
 
     // If inlining fails, the compiler will issue an inliner warning that is not present in the
     // check file
-    compileString(newCompiler("-usejavacp", "-optimise"))(bCode)
+    compileString(newCompiler("-usejavacp", "-optimise", "-Ybackend:GenASM"))(bCode)
   }
 
   def readClass(file: String) = {

test/files/run/elidable-opt.scala

@@ -28,7 +28,7 @@ enterStat(<synthetic> val C$1: C = x$1.asInstanceOf[C])
 enterSym(def <init>() = {   super.<init>();   () })
 enterSym(final override <synthetic> def toString() = "C")
 enterSym(case <synthetic> def apply(x: Int, y: Int): C = new C(x, y))
-enterSym(case <synthetic> def unapply(x$0: C) = if (x$0.==(null))   scala.this.None else   Some(scala.Tuple2(x$0.x, x$0.y)))
+enterSym(case <synthetic> def unapply(x$0: C): _root_.scala.Option[scala.Tuple2[Int, Int]] = if (x$0.==(null))   scala.this.None else   Some(scala.Tuple2(x$0.x, x$0.y)))
 enterStat(def <init>() = {   super.<init>();   () })
 enterStat(final override <synthetic> def toString() = "C")
 enterSym(def <init>() = {   super.<init>();   () })

test/files/run/finalvar.flags

@@ -1 +1 @@
--optimise
+-optimise -Ybackend:GenASM

test/files/run/icode-reader-dead-code.scala

@@ -1 +1 @@
--optimize
+-optimize -Ybackend:GenASM