Improve inlining heuristics.

pkg/compiler/lib/src/ssa/builder.dart

Index: pkg/compiler/lib/src/ssa/builder.dart
diff --git a/pkg/compiler/lib/src/ssa/builder.dart b/pkg/compiler/lib/src/ssa/builder.dart
index aebf9b8ad7bb34d13b71a089ff78c5935a967a1f..88ec9e858403ce34ee47150935d424e75a793b7d 100644
--- a/pkg/compiler/lib/src/ssa/builder.dart
+++ b/pkg/compiler/lib/src/ssa/builder.dart
@@ -478,7 +478,7 @@ class SsaBuilder extends ast.Visitor
 
     bool doesNotContainCode() {
       // A function with size 1 does not contain any code.
-      return InlineWeeder.canBeInlined(functionResolvedAst, 1, true,
+      return InlineWeeder.canBeInlined(elements, functionResolvedAst, 1, true,
           enableUserAssertions: compiler.options.enableUserAssertions);
     }
 
@@ -486,8 +486,8 @@ class SsaBuilder extends ast.Visitor
       // The call is on a path which is executed rarely, so inline only if it
       // does not make the program larger.
       if (isCalledOnce(element)) {
-        return InlineWeeder.canBeInlined(functionResolvedAst, -1, false,
-            enableUserAssertions: compiler.options.enableUserAssertions);
+        return InlineWeeder.canBeInlined(elements, functionResolvedAst, -1,
+            false, enableUserAssertions: compiler.options.enableUserAssertions);
       }
       // TODO(sra): Measure if inlining would 'reduce' the size.  One desirable
       // case we miss by doing nothing is inlining very simple constructors
@@ -524,8 +524,8 @@ class SsaBuilder extends ast.Visitor
       if (cachedCanBeInlined == true) {
         // We may have forced the inlining of some methods. Therefore check
         // if we can inline this method regardless of size.
-        assert(InlineWeeder.canBeInlined(functionResolvedAst, -1, false,
-            allowLoops: true,
+        assert(InlineWeeder.canBeInlined(elements, functionResolvedAst, -1,
+            false, allowLoops: true,
             enableUserAssertions: compiler.options.enableUserAssertions));
         return true;
       }
@@ -549,6 +549,7 @@ class SsaBuilder extends ast.Visitor
       }
       bool canInline;
       canInline = InlineWeeder.canBeInlined(
+          elements,
           functionResolvedAst, maxInliningNodes, useMaxInliningNodes,
           enableUserAssertions: compiler.options.enableUserAssertions);
       if (canInline) {
@@ -6616,18 +6617,24 @@ class InlineWeeder extends ast.Visitor {
   final bool allowLoops;
   final bool enableUserAssertions;
 
-  InlineWeeder(this.maxInliningNodes, this.useMaxInliningNodes, this.allowLoops,
+  final TreeElements elements;
+
+  InlineWeeder(this.elements,
+      this.maxInliningNodes, this.useMaxInliningNodes, this.allowLoops,
       this.enableUserAssertions);
 
   static bool canBeInlined(
+      TreeElements elements,
       ResolvedAst resolvedAst, int maxInliningNodes, bool useMaxInliningNodes,
       {bool allowLoops: false, bool enableUserAssertions: null}) {
     assert(enableUserAssertions is bool); // Ensure we passed it.
     if (resolvedAst.elements.containsTryStatement) return false;

[sra1, 2017/02/28 22:48:28]

elements does not need to be passed in, since it it resolvedAst.elements

[floitsch, 2017/03/01 08:50:45]

even better.

 
-    InlineWeeder weeder = new InlineWeeder(maxInliningNodes,
+    InlineWeeder weeder = new InlineWeeder(elements,
+        maxInliningNodes,
         useMaxInliningNodes, allowLoops, enableUserAssertions);
     ast.FunctionExpression functionExpression = resolvedAst.node;
+
     weeder.visit(functionExpression.initializers);
     weeder.visit(functionExpression.body);
     weeder.visit(functionExpression.asyncModifier);
@@ -6682,6 +6689,14 @@ class InlineWeeder extends ast.Visitor {
   }
 
   void visitSend(ast.Send node) {
+    /*

[floitsch, 2016/12/21 10:36:24]

Enabling this, slightly increases the output size.

I think we can play a bit with the max-sizes, though. If we count references to
the arguments as less important, then we can try to decrease the overall
inlining size.

+    Element element = elements[node];
+    if (element != null && element.isParameter) {
+      // Don't count as additional node, since it's likely that passing the
+      // argument would cost us as much space as we inline.

[sra1, 2017/02/28 19:36:15]

It will cost a temporary if it is used twice.
Can we count uses?

[floitsch, 2017/03/01 08:50:45]

That's true, unless the parameter is a constant or already in a temporary.
I do agree that we shouldn't just ignore sends to arguments, *but* they should
be counted less than other variables. Lots of chances of optimizations when one
can inline declarations to their uses.

+      return;
+    }
+    */
     if (!registerNode()) return;
     node.visitChildren(this);
   }
@@ -6698,6 +6713,26 @@ class InlineWeeder extends ast.Visitor {
     tooDifficult = true;
   }
 
+  void visitConditional(ast.Conditional node) {
+    visit(node.condition);
+    if (tooDifficult) return;
+    int oldCount = nodeCount;

[sra1, 2017/02/28 19:36:15]

I think this code would be easier to understand if this was called
commonPrefixCount, and the then/else were explicitly computed.

[floitsch, 2017/03/01 08:50:45]

Acknowledged.

+    visit(node.thenExpression);
+    if (tooDifficult) return;
+    int thenCount = nodeCount;

[sra1, 2017/02/28 19:36:15]

int thenCount = nodeCount - commonPrefixCount
...
int elseCount = nodeCount - commonPrefixCount


nodeCount = commonPrefixCount + max(thenCount, elseCount);
...
nodeCount = commonPrefixCount + thenCount + elseCount;

[floitsch, 2017/03/01 08:50:45]

Acknowledged.

+    nodeCount = oldCount;
+    visit(node.elseExpression);
+    if (tooDifficult) return;
+    if (node.condition.asSend() != null &&

[sra1, 2017/02/28 19:36:15]

Add a comment that this is a heuristic - a parameter directly used in a
conditional is likely to be a constant, so count only the biggest arm.

[floitsch, 2017/03/01 08:50:45]

Acknowledged.

+        elements[node.condition]?.isParameter == true) {
+      nodeCount = thenCount > nodeCount ? thenCount : nodeCount;
+    } else {
+      nodeCount += (thenCount - oldCount);
+    }
+    if (!registerNode()) return;
+
+  }
+
   void visitRethrow(ast.Rethrow node) {
     if (!registerNode()) return;
     tooDifficult = true;