Fixed a bug in the optimizer which was preventi...

Fixed a bug in the optimizer which was preventing private methods from
being inlined. Also relaxes a condition related to the "liftedTry"
problem: the inliner has to exclude certain methods from consideration
if there is a value on the stack and the method being inlined has
exception handlers. The new condition is as before, except that it does
not exclude methods of the "try/finally" variety (i.e. finalizers, but
no other exception handlers.) This is necessary to optimize this common
pattern:

  @inline private def foo(body: => Unit) {
    val saved = something
    try body
    finally something = saved
  }

The closure for "body" can be fully eliminated, but only if the contents
of foo can be inlined into the caller.

Closes #4764, review by rompf.

13 files changed, 163 insertions, 58 deletions

diff --git a/src/compiler/scala/tools/nsc/Global.scala b/src/compiler/scala/tools/nsc/Global.scala
index 2cf8246db1..d48b6fa569 100644
--- a/src/compiler/scala/tools/nsc/Global.scala
+++ b/src/compiler/scala/tools/nsc/Global.scala
@@ -163,6 +163,14 @@ class Global(var currentSettings: Settings, var reporter: Reporter) extends Symb
     if (opt.fatalWarnings) globalError(msg)
     else reporter.warning(NoPosition, msg)
 
+  @inline final def ifDebug(body: => Unit) {
+    if (settings.debug.value)
+      body
+  }
+  @inline final def debuglog(msg: => String) {
+    if (settings.debug.value)
+      inform("[log " + phase + "] " + msg)
+  }
   private def elapsedMessage(msg: String, start: Long) =
     msg + " in " + (currentTime - start) + "ms"
 
diff --git a/src/compiler/scala/tools/nsc/backend/icode/GenICode.scala b/src/compiler/scala/tools/nsc/backend/icode/GenICode.scala
index d54100b53a..f615b5fc13 100644
--- a/src/compiler/scala/tools/nsc/backend/icode/GenICode.scala
+++ b/src/compiler/scala/tools/nsc/backend/icode/GenICode.scala
@@ -39,9 +39,6 @@ abstract class GenICode extends SubComponent  {
 
   override def newPhase(prev: Phase) = new ICodePhase(prev)
 
-  private def debugLog(msg: => String): Unit =
-    if (settings.debug.value) log(msg)
-
   class ICodePhase(prev: Phase) extends StdPhase(prev) {
 
     override def description = "Generate ICode from the AST"
@@ -316,7 +313,7 @@ abstract class GenICode extends SubComponent  {
         MONITOR_ENTER() setPos tree.pos
       ))
       ctx1.enterSynchronized(monitor)
-      debugLog("synchronized block start")
+      debuglog("synchronized block start")
 
       ctx1 = ctx1.Try(
         bodyCtx => {
@@ -340,7 +337,7 @@ abstract class GenICode extends SubComponent  {
             exhCtx
           })), EmptyTree, tree)
 
-      debugLog("synchronized block end with block %s closed=%s".format(ctx1.bb, ctx1.bb.closed))
+      debuglog("synchronized block end with block %s closed=%s".format(ctx1.bb, ctx1.bb.closed))
       ctx1.exitSynchronized(monitor)
       if (hasResult)
         ctx1.bb.emit(LOAD_LOCAL(monitorResult))
@@ -368,7 +365,7 @@ abstract class GenICode extends SubComponent  {
       val resKind = if (hasUnitBranch) UNIT else ifKind
 
       if (hasUnitBranch)
-        debugLog("Will drop result from an if branch")
+        debuglog("Will drop result from an if branch")
 
       thenCtx = genLoad(thenp, thenCtx, resKind)
       elseCtx = genLoad(elsep, elseCtx, resKind)
diff --git a/src/compiler/scala/tools/nsc/backend/icode/analysis/ReachingDefinitions.scala b/src/compiler/scala/tools/nsc/backend/icode/analysis/ReachingDefinitions.scala
index 31a2dbfbec..cc4619c68f 100644
--- a/src/compiler/scala/tools/nsc/backend/icode/analysis/ReachingDefinitions.scala
+++ b/src/compiler/scala/tools/nsc/backend/icode/analysis/ReachingDefinitions.scala
@@ -241,12 +241,10 @@ abstract class ReachingDefinitions {
       findDefs(bb, idx, m, 0)
 
     override def toString: String = {
-      val sb = new StringBuilder
-      sb.append("rdef: \n")
-      for (b <- method.code.blocks)
-        sb.append("rdef_entry(" + b + ")= " + in(b)).append("\nrdef_exit(" + b + ")= " + out(b))
-      sb.toString()
+      method.code.blocks map { b =>
+        "  entry(%s) = %s\n".format(b, in(b)) +
+        "   exit(%s) = %s\n".format(b, out(b))
+      } mkString ("ReachingDefinitions {\n", "\n", "\n}")
     }
-
   }
 }
diff --git a/src/compiler/scala/tools/nsc/backend/icode/analysis/SemiLattice.scala b/src/compiler/scala/tools/nsc/backend/icode/analysis/SemiLattice.scala
index d458b2546f..cfce0878ed 100644
--- a/src/compiler/scala/tools/nsc/backend/icode/analysis/SemiLattice.scala
+++ b/src/compiler/scala/tools/nsc/backend/icode/analysis/SemiLattice.scala
@@ -3,7 +3,6 @@
  * @author  Martin Odersky
  */
 
-
 package scala.tools.nsc
 package backend.icode
 package analysis
@@ -26,6 +25,11 @@ trait SemiLattice {
       case _ =>
         false
     }
+    private def tstring(x: Any): String = x match {
+      case xs: TraversableOnce[_] => xs map tstring mkString " "
+      case _                      => "" + x
+    }
+    override def toString = "IState(" + tstring(vars) + ", " + tstring(stack) + ")"
   }
 
   /** Return the least upper bound of a and b. */
diff --git a/src/compiler/scala/tools/nsc/backend/opt/Inliners.scala b/src/compiler/scala/tools/nsc/backend/opt/Inliners.scala
index 3f1e15ba0d..07115e28ca 100644
--- a/src/compiler/scala/tools/nsc/backend/opt/Inliners.scala
+++ b/src/compiler/scala/tools/nsc/backend/opt/Inliners.scala
@@ -98,8 +98,7 @@ abstract class Inliners extends SubComponent {
 
     def analyzeClass(cls: IClass): Unit =
       if (settings.inline.value) {
-        if (settings.debug.value)
-        	log("Analyzing " + cls)
+        debuglog("Analyzing " + cls)
 
         this.currentIClazz = cls
         cls.methods filterNot (_.symbol.isConstructor) foreach analyzeMethod
@@ -126,9 +125,10 @@ abstract class Inliners extends SubComponent {
       def analyzeInc(msym: Symbol, i: Instruction, bb: BasicBlock): Boolean = {
         var inlined = false
         def paramTypes  = msym.info.paramTypes
-        val receiver    = (info.stack.types drop paramTypes.length).head match {
-          case REFERENCE(s) => s
-          case _            => NoSymbol
+        val receiver    = (info.stack.types drop paramTypes.length) match {
+          case Nil               => log("analyzeInc(" + msym + "), no type on the stack!") ; NoSymbol
+          case REFERENCE(s) :: _ => s
+          case _                 => NoSymbol
         }
         val concreteMethod  = lookupImplFor(msym, receiver)
 
@@ -149,8 +149,7 @@ abstract class Inliners extends SubComponent {
                 || receiver.enclosingPackage == definitions.RuntimePackage
                 )   // only count non-closures
 
-        if (settings.debug.value)
-          log("Treating " + i
+        debuglog("Treating " + i
               + "\n\treceiver: " + receiver
               + "\n\ticodes.available: " + isAvailable
               + "\n\tconcreteMethod.isEffectivelyFinal: " + concreteMethod.isEffectivelyFinal)
@@ -185,8 +184,7 @@ abstract class Inliners extends SubComponent {
               }
             case None =>
               warnNoInline("bytecode was not available")
-              if (settings.debug.value)
-                log("could not find icode\n\treceiver: " + receiver + "\n\tmethod: " + concreteMethod)
+              debuglog("could not find icode\n\treceiver: " + receiver + "\n\tmethod: " + concreteMethod)
           }
         }
         else warnNoInline(
@@ -212,8 +210,11 @@ abstract class Inliners extends SubComponent {
             breakable {
               for (i <- bb) {
                 i match {
-                  case CALL_METHOD(msym, Dynamic) =>
-                    if (analyzeInc(msym, i, bb)) break
+                  // Dynamic == normal invocations
+                  // Static(true) == calls to private members
+                  case CALL_METHOD(msym, Dynamic | Static(true)) if !msym.isConstructor =>
+                    if (analyzeInc(msym, i, bb))
+                      break
                   case _ => ()
                 }
                 info = tfa.interpret(info, i)
@@ -253,8 +254,7 @@ abstract class Inliners extends SubComponent {
       def loadCondition = sym.isEffectivelyFinal && isMonadicMethod(sym) && isHigherOrderMethod(sym)
 
       val res = hasInline(sym) || alwaysLoad || loadCondition
-      if (settings.debug.value)
-        log("shouldLoadImplFor: " + receiver + "." + sym + ": " + res)
+      debuglog("shouldLoadImplFor: " + receiver + "." + sym + ": " + res)
       res
     }
 
@@ -274,8 +274,7 @@ abstract class Inliners extends SubComponent {
       }
       if (needsLookup) {
         val concreteMethod = lookup(clazz)
-        if (settings.debug.value)
-          log("\tlooked up method: " + concreteMethod.fullName)
+        debuglog("\tlooked up method: " + concreteMethod.fullName)
 
         concreteMethod
       }
@@ -312,6 +311,10 @@ abstract class Inliners extends SubComponent {
       def hasCode       = m.code != null
       def hasSourceFile = m.sourceFile != null
       def hasHandlers   = handlers.nonEmpty
+      def hasNonFinalizerHandler = handlers exists {
+        case _: Finalizer => true
+        case _            => false
+      }
 
       // the number of inlined calls in 'm', used by 'shouldInline'
       var inlinedCalls = 0
@@ -535,8 +538,7 @@ abstract class Inliners extends SubComponent {
       def isSafeToInline(stack: TypeStack): Boolean = {
         def makePublic(f: Symbol): Boolean =
           inc.hasSourceFile && (f.isSynthetic || f.isParamAccessor) && {
-            if (settings.debug.value)
-              log("Making not-private symbol out of synthetic: " + f)
+            debuglog("Making not-private symbol out of synthetic: " + f)
 
             f setNotFlag Flags.PRIVATE
             true
@@ -572,22 +574,20 @@ abstract class Inliners extends SubComponent {
           }
 
           def iterate(): NonPublicRefs.Value = inc.instructions.foldLeft(Public)((res, inc) => getAccess(inc) match {
-            case Private    => return Private
+            case Private    => log("instruction " + inc + " requires private access.") ; return Private
             case Protected  => Protected
             case Public     => res
           })
           iterate()
         })
 
-        def isIllegalStack = (stack.length > inc.minimumStack && inc.hasHandlers) || {
-          if (settings.debug.value)
-            log("method " + inc.sym + " is used on a non-empty stack with finalizer.")
-
-          false
+        canAccess(accessNeeded) && {
+          val isIllegalStack = (stack.length > inc.minimumStack && inc.hasNonFinalizerHandler)
+          !isIllegalStack || {
+            debuglog("method " + inc.sym + " is used on a non-empty stack with finalizer.  Stack: " + stack)
+            false
+          }
         }
-//        if (!canAccess(accessNeeded))
-//          println("access needed and failed: " + accessNeeded)
-        canAccess(accessNeeded) && !isIllegalStack
       }
 
       /** Decide whether to inline or not. Heuristics:
@@ -601,8 +601,7 @@ abstract class Inliners extends SubComponent {
       private def alwaysInline  = inc.inline
 
       def shouldInline: Boolean = !neverInline && (alwaysInline || {
-        if (settings.debug.value)
-          log("shouldInline: " + caller.m + " with " + inc.m)
+        debuglog("shouldInline: " + caller.m + " with " + inc.m)
 
         var score = 0
 
@@ -615,8 +614,7 @@ abstract class Inliners extends SubComponent {
           score += 1
         if (caller.isSmall && isLargeSum) {
           score -= 1
-          if (settings.debug.value)
-            log("shouldInline: score decreased to " + score + " because small " + caller + " would become large")
+          debuglog("shouldInline: score decreased to " + score + " because small " + caller + " would become large")
         }
         if (inc.isLarge)
           score -= 1
diff --git a/src/compiler/scala/tools/nsc/matching/MatchSupport.scala b/src/compiler/scala/tools/nsc/matching/MatchSupport.scala
index beaf63106d..91e139381e 100644
--- a/src/compiler/scala/tools/nsc/matching/MatchSupport.scala
+++ b/src/compiler/scala/tools/nsc/matching/MatchSupport.scala
@@ -95,8 +95,6 @@ trait MatchSupport extends ast.TreeDSL { self: ParallelMatching =>
       })
     }
 
-    @elidable(elidable.FINE) def ifDebug(body: => Unit): Unit     = { if (settings.debug.value) body }
-    @elidable(elidable.FINE) def DBG(msg: => String): Unit        = { ifDebug(println(msg)) }
     @elidable(elidable.FINE) def TRACE(f: String, xs: Any*): Unit = {
       if (trace) {
         val msg = if (xs.isEmpty) f else f.format(xs map pp: _*)
diff --git a/src/compiler/scala/tools/nsc/symtab/classfile/ClassfileParser.scala b/src/compiler/scala/tools/nsc/symtab/classfile/ClassfileParser.scala
index 84054680c4..a0654395d2 100644
--- a/src/compiler/scala/tools/nsc/symtab/classfile/ClassfileParser.scala
+++ b/src/compiler/scala/tools/nsc/symtab/classfile/ClassfileParser.scala
@@ -1327,10 +1327,7 @@ abstract class ClassfileParser {
         sym.privateWithin = sym.toplevelClass.owner
   }
 
-  @inline final private def isPrivate(flags: Int) =
-    (flags & JAVA_ACC_PRIVATE) != 0
-  @inline final private def isStatic(flags: Int) =
-    (flags & JAVA_ACC_STATIC) != 0
-  @inline final private def hasAnnotation(flags: Int) =
-    (flags & JAVA_ACC_ANNOTATION) != 0
+  @inline private def isPrivate(flags: Int)     = (flags & JAVA_ACC_PRIVATE) != 0
+  @inline private def isStatic(flags: Int)      = (flags & JAVA_ACC_STATIC) != 0
+  @inline private def hasAnnotation(flags: Int) = (flags & JAVA_ACC_ANNOTATION) != 0
 }
diff --git a/src/compiler/scala/tools/nsc/transform/SpecializeTypes.scala b/src/compiler/scala/tools/nsc/transform/SpecializeTypes.scala
index ced2874a14..7dd72e0d10 100644
--- a/src/compiler/scala/tools/nsc/transform/SpecializeTypes.scala
+++ b/src/compiler/scala/tools/nsc/transform/SpecializeTypes.scala
@@ -54,13 +54,6 @@ abstract class SpecializeTypes extends InfoTransform with TypingTransformers {
     case _                                        => false
   }
 
-  @inline private def debuglog(msg: => String) {
-    if (settings.debug.value) log(msg)
-  }
-  @inline private def ifDebug(body: => Unit) {
-    if (settings.debug.value) { body }
-  }
-
   object TypeEnv {
     /** Return a new type environment binding specialized type parameters of sym to
      *  the given args. Expects the lists to have the same length.
diff --git a/test/files/pos/inliner2.flags b/test/files/pos/inliner2.flags
new file mode 100644
index 0000000000..ea03113c66
--- /dev/null
+++ b/test/files/pos/inliner2.flags
@@ -0,0 +1 @@
+-optimise -Xfatal-warnings
\ No newline at end of file
diff --git a/test/files/pos/inliner2.scala b/test/files/pos/inliner2.scala
new file mode 100644
index 0000000000..bc83e04312
--- /dev/null
+++ b/test/files/pos/inliner2.scala
@@ -0,0 +1,57 @@
+// This isn't actually testing much, because no warning is emitted in versions
+// before the fix which comes with this because the method isn't even considered
+// for inlining due to the bug.
+class A {
+  private var debug = false
+  @inline private def ifelse[T](cond: => Boolean, ifPart: => T, elsePart: => T): T =
+    if (cond) ifPart else elsePart
+
+  final def bob1() = ifelse(debug, 1, 2)
+  final def bob2() = if (debug) 1 else 2
+}
+// Cool:
+//
+// % ls -1 /tmp/2901/
+// A$$anonfun$bob1$1.class
+// A$$anonfun$bob1$2.class
+// A$$anonfun$bob1$3.class
+// A.class
+// % ls -1 /tmp/trunk
+// A.class
+//
+// Observations:
+//
+// (1) The inlined version accesses the field: the explicit one calls the accessor.
+// (2) The inlined version fails to eliminate boxing.  With reference types it emits
+//     an unneeded checkcast.
+// (3) The private var debug is mangled to A$$debug, but after inlining it is never accessed
+//     from outside of the class and doesn't need mangling.
+// (4) We could forego emitting bytecode for ifelse entirely if it has been
+//     inlined at all sites.
+//
+// Generated bytecode for the above:
+//
+// public final int bob1();
+//   Code:
+//    Stack=1, Locals=1, Args_size=1
+//    0:  aload_0
+//    1:  getfield  #11; //Field A$$debug:Z
+//    4:  ifeq  14
+//    7:  iconst_1
+//    8:  invokestatic  #41; //Method scala/runtime/BoxesRunTime.boxToInteger:(I)Ljava/lang/Integer;
+//    11: goto  18
+//    14: iconst_2
+//    15: invokestatic  #41; //Method scala/runtime/BoxesRunTime.boxToInteger:(I)Ljava/lang/Integer;
+//    18: invokestatic  #45; //Method scala/runtime/BoxesRunTime.unboxToInt:(Ljava/lang/Object;)I
+//    21: ireturn
+//
+// public final int bob2();
+//   Code:
+//    Stack=1, Locals=1, Args_size=1
+//    0:  aload_0
+//    1:  invokevirtual #48; //Method A$$debug:()Z
+//    4:  ifeq  11
+//    7:  iconst_1
+//    8:  goto  12
+//    11: iconst_2
+//    12: ireturn
diff --git a/test/files/run/private-inline.check b/test/files/run/private-inline.check
new file mode 100644
index 0000000000..209e3ef4b6
--- /dev/null
+++ b/test/files/run/private-inline.check
@@ -0,0 +1 @@
+20
diff --git a/test/files/run/private-inline.flags b/test/files/run/private-inline.flags
new file mode 100644
index 0000000000..eb4d19bcb9
--- /dev/null
+++ b/test/files/run/private-inline.flags
@@ -0,0 +1 @@
+-optimise
\ No newline at end of file
diff --git a/test/files/run/private-inline.scala b/test/files/run/private-inline.scala
new file mode 100644
index 0000000000..5181ac8ba3
--- /dev/null
+++ b/test/files/run/private-inline.scala
@@ -0,0 +1,52 @@
+
+final class A {
+  private var x1 = false
+  var x2 = false
+
+  // manipulates private var
+  @inline private def wrapper1[T](body: => T): T = {
+    val saved = x1
+    x1 = true
+    try body
+    finally x1 = saved
+  }
+  // manipulates public var
+  @inline private def wrapper2[T](body: => T): T = {
+    val saved = x2
+    x2 = true
+    try body
+    finally x2 = saved
+  }
+
+  // not inlined
+  def f1a() = wrapper1(5)
+  // inlined!
+  def f1b() = identity(wrapper1(5))
+
+  // not inlined
+  def f2a() = wrapper2(5)
+  // inlined!
+  def f2b() = identity(wrapper2(5))
+}
+
+object Test {
+  def methodClasses = List("f1a", "f1b", "f2a", "f2b") map ("A$$anonfun$" + _ + "$1")
+
+  def main(args: Array[String]): Unit = {
+    val a = new A
+    import a._
+    println(f1a() + f1b() + f2a() + f2b())
+
+    // Don't know how else to test this: all these should have been
+    // inlined, so all should fail.
+    methodClasses foreach { clazz =>
+
+      val foundClass = (
+        try   Class.forName(clazz)
+        catch { case _ => null }
+      )
+
+      assert(foundClass == null, foundClass)
+    }
+  }
+}