AOT: Use a cid range check when possible to implement type tests.

AOT: Use a cid range check when possible to implement type tests.

R=fschneider@google.com

Committed: https://github.com/dart-lang/sdk/commit/6137d020b96a4d46de9de2cc357295d52a6a09f2

[rmacnak, 2016-09-07 01:20:26]

rmacnak@google.com changed reviewers:
+ fschneider@google.com, regis@google.com

[rmacnak, 2016-09-07 01:20:28]

https://codereview.chromium.org/2314133003/diff/20001/runtime/vm/flow_graph.h
File runtime/vm/flow_graph.h (right):

https://codereview.chromium.org/2314133003/diff/20001/runtime/vm/flow_graph.h...
runtime/vm/flow_graph.h:318: friend class AotOptimizer;
ComputeDominators

https://codereview.chromium.org/2314133003/diff/40001/runtime/vm/precompiler.cc
File runtime/vm/precompiler.cc (right):

https://codereview.chromium.org/2314133003/diff/40001/runtime/vm/precompiler....
runtime/vm/precompiler.cc:243: cls_type = cls.DeclarationType();
Regis, should I used DeclarationType or RareType?

[Florian Schneider, 2016-09-07 16:35:57]

https://codereview.chromium.org/2314133003/diff/40001/runtime/vm/aot_optimize...
File runtime/vm/aot_optimizer.cc (right):

https://codereview.chromium.org/2314133003/diff/40001/runtime/vm/aot_optimize...
runtime/vm/aot_optimizer.cc:1635: void
AotOptimizer::ReplaceWithClassRangeCheck(InstanceCallInstr* call,
I wonder if it would be easier to replace the call with another call to a
internal static top-level Dart function:

bool _classRangeCheck(int cid, int lower, int upper) {
  return cid >= lower && cid <= upper;
}

(or the negated version of it)

If you add this method to the always-inline list, the result after inlining
should be the same. The advantage would be that only straight-line code has to
be inserted into the graph (LoadClassId followed by StaticCall). What do you
think?

[regis, 2016-09-07 17:06:08]

https://codereview.chromium.org/2314133003/diff/40001/runtime/vm/precompiler.cc
File runtime/vm/precompiler.cc (right):

https://codereview.chromium.org/2314133003/diff/40001/runtime/vm/precompiler....
runtime/vm/precompiler.cc:243: cls_type = cls.DeclarationType();
On 2016/09/07 01:20:27, rmacnak wrote:
> Regis, should I used DeclarationType or RareType?

RareType, because DeclarationType would yield an uninstantiated type for a
generic class. And you cannot have a left-hand side uninstantiated type in a
type test.

[rmacnak, 2016-09-07 22:21:28]

https://chromiumcodereview.appspot.com/2314133003/diff/40001/runtime/vm/aot_o...
File runtime/vm/aot_optimizer.cc (right):

https://chromiumcodereview.appspot.com/2314133003/diff/40001/runtime/vm/aot_o...
runtime/vm/aot_optimizer.cc:1635: void
AotOptimizer::ReplaceWithClassRangeCheck(InstanceCallInstr* call,
On 2016/09/07 16:35:57, Florian Schneider wrote:
> I wonder if it would be easier to replace the call with another call to a
> internal static top-level Dart function:
> 
> bool _classRangeCheck(int cid, int lower, int upper) {
>   return cid >= lower && cid <= upper;
> }
> 
> (or the negated version of it)
> 
> If you add this method to the always-inline list, the result after inlining
> should be the same. The advantage would be that only straight-line code has to
> be inserted into the graph (LoadClassId followed by StaticCall). What do you
> think?

Yes, I like that better.

https://chromiumcodereview.appspot.com/2314133003/diff/40001/runtime/vm/preco...
File runtime/vm/precompiler.cc (right):

https://chromiumcodereview.appspot.com/2314133003/diff/40001/runtime/vm/preco...
runtime/vm/precompiler.cc:243: cls_type = cls.DeclarationType();
On 2016/09/07 17:06:07, regis wrote:
> On 2016/09/07 01:20:27, rmacnak wrote:
> > Regis, should I used DeclarationType or RareType?
> 
> RareType, because DeclarationType would yield an uninstantiated type for a
> generic class. And you cannot have a left-hand side uninstantiated type in a
> type test.

Done.

[rmacnak, 2016-09-07 22:26:26]

Flutter gallery 
VMIsolate(CodeSize): 116350
Isolate(CodeSize): 2717811
ReadOnlyData(CodeSize): 1714744
Instructions(CodeSize): 5556384
Total(CodeSize): 10105289
 =>
VMIsolate(CodeSize): 116358
Isolate(CodeSize): 2696463
ReadOnlyData(CodeSize): 1713080
Instructions(CodeSize): 5489008 -1.21%
Total(CodeSize): 10014909 -0.89%

Type check for Type: class 'String' is cid range [70, 73]
Type check for Type: class 'double' is cid range [50, 50]
Type check for Type: class 'InternalStyle' is cid range [192, 194]
Type check for Type: class 'RenderObject' is cid range [1029, 1126]
Type check for Type: class 'UnsupportedError' is cid range [2875, 2876]
Type check for Type: class 'OffsetBase' is cid range [2197, 2199]
Type check for Type: class 'Size' is cid range [2197, 2198]
Type check for Type: class 'InternetAddress' is cid range [2209, 2209]
Type check for Type: class 'TlsException' is cid range [2249, 2251]
Type check for Type: class 'HttpResponse' is cid range [2274, 2274]
Type check for Type: class 'ArgumentError' is cid range [2880, 2882]
Type check for Type: class '_ControllerStream@6048458' is cid range [2750, 2751]
Type check for Type: class 'GlobalKey' is cid range [847, 849]
Type check for Type: class 'RenderObjectElement' is cid range [348, 361]
Type check for Type: class 'RenderBox' is cid range [1029, 1124]
Type check for Type: class 'TextStyle' is cid range [1146, 1148]
Type check for Type: class 'TransitionRoute' is cid range [306, 316]
Type check for Type: class 'ParentData' is cid range [997, 1011]
Type check for Type: class 'BoxParentData' is cid range [998, 1011]
Type check for Type: class 'MaterialPageRoute' is cid range [309, 311]
Type check for Type: class 'PageRoute' is cid range [309, 311]
Type check for Type: class 'TextSpan' is cid range [1144, 1145]
Type check for Type: class 'TextRange' is cid range [1149, 1150]
Type check for Type: class 'ModalRoute' is cid range [309, 316]
Type check for Type: class 'ScrollableState' is cid range [477, 483]
Type check for Type: class 'SourceSpan' is cid range [184, 186]
Type check for Type: class 'FileSpan' is cid range [184, 184]
Type check for Type: class 'SourceLocation' is cid range [179, 181]

[rmacnak, 2016-09-07 22:28:33]

dart2js
VMIsolate(CodeSize):     173958
Isolate(CodeSize):      4705941
ReadOnlyData(CodeSize): 4056568
Instructions(CodeSize): 9039104
Total(CodeSize):       17975571 

VMIsolate(CodeSize):     174010
Isolate(CodeSize):      4671558
ReadOnlyData(CodeSize): 4022088
Instructions(CodeSize): 8912928 -1.39%
Total(CodeSize):       17780584 -1.08%

Type check for Type: class 'String' is cid range [70, 73]
Type check for Type: class 'InternalStyle' is cid range [1859, 1861]
Type check for Type: class 'double' is cid range [50, 50]
Type check for Type: class 'Future' is cid range [2745, 2745]
Type check for Type: class 'ErrorToken' is cid range [1711, 1713]
Type check for Type: class 'File' is cid range [2327, 2327]
Type check for Type: class 'UnsupportedError' is cid range [2881, 2882]
Type check for Type: class 'Element' is cid range [1897, 2102]
Type check for Type: class 'HInstruction' is cid range [1421, 1513]
Type check for Type: class 'Node' is cid range [1242, 1327]
Type check for Type: class 'Token' is cid range [1709, 1716]
Type check for Type: class 'CallSiteTypeInformation' is cid range [636, 638]
Type check for Type: class 'CallStructure' is cid range [1883, 1884]
Type check for Type: class 'PublicName' is cid range [1695, 1696]
Type check for Type: class 'InternetAddress' is cid range [2194, 2194]
Type check for Type: class 'ArgumentError' is cid range [2886, 2888]
Type check for Type: class 'DartType' is cid range [1748, 1759]
Type check for Type: class 'GenericType' is cid range [1751, 1754]
Type check for Type: class 'InterfaceType' is cid range [1753, 1754]
Type check for Type: class 'TlsException' is cid range [2234, 2236]
Type check for Type: class 'HttpResponse' is cid range [2259, 2259]
Type check for Type: class '_ControllerStream@6048458' is cid range [2741, 2742]
Type check for Type: class 'Loop' is cid range [1288, 1293]
Type check for Type: class 'Expression' is cid range [462, 520]
Type check for Type: class 'Node' is cid range [432, 520]
Type check for Type: class 'LiteralString' is cid range [497, 498]
Type check for Type: class 'Name' is cid range [501, 508]
Type check for Type: class 'Assignment' is cid range [486, 487]
Type check for Type: class 'HLocalValue' is cid range [1443, 1445]
Type check for Type: class 'ErroneousElement' is cid range [2029, 2033]
Type check for Type: class 'PrimitiveConstantValue' is cid range [1634, 1641]
Type check for Type: class 'TypeVariableType' is cid range [1758, 1759]
Type check for Type: class 'ConstantExpression' is cid range [1768, 1795]
Type check for Type: class '_Zone@6048458' is cid range [2641, 2642]
Type check for Type: class 'Name' is cid range [297, 298]
Type check for Type: class 'Member' is cid range [387, 389]
Type check for Type: class 'HParameterValue' is cid range [1444, 1445]
Type check for Type: class 'HCheck' is cid range [1505, 1508]
Type check for Type: class 'HForeign' is cid range [1471, 1471]
Type check for Type: class 'ConstructorBodyElement' is cid range [2037, 2037]
Type check for Type: class 'Identifier' is cid range [1314, 1315]
Type check for Type: class 'TypedefType' is cid range [1751, 1752]
Type check for Type: class 'Send' is cid range [1317, 1318]
Type check for Type: class 'Statement' is cid range [438, 459]
Type check for Type: class 'JsonUnsupportedObjectError' is cid range [2873,
2874]
Type check for Type: class 'Class' is cid range [732, 733]
Type check for Type: class 'ForwardingTypeMask' is cid range [892, 895]
Type check for Type: class 'MapTypeMask' is cid range [893, 894]
Type check for Type: class 'HRelational' is cid range [1453, 1457]
Type check for Type: class 'InstructionValue' is cid range [278, 279]
Type check for Type: class 'HInvokeDynamic' is cid range [1483, 1488]
Type check for Type: class 'HInvoke' is cid range [1479, 1488]
Type check for Type: class 'HInvokeStatic' is cid range [1479, 1481]
Type check for Type: class 'Utf8BytesSourceFile' is cid range [2172, 2173]
Type check for Type: class '_PrefixedName@158437770' is cid range [505, 507]
Type check for Type: class 'Member' is cid range [705, 708]
Type check for Type: class 'DeclaredMember' is cid range [705, 708]
Type check for Type: class 'HConditionalBranch' is cid range [1502, 1503]
Type check for Type: class 'HControlFlow' is cid range [1491, 1503]
Type check for Type: class 'HJump' is cid range [1496, 1497]
Type check for Type: class 'MapConstantValue' is cid range [1629, 1630]
Type check for Type: class 'SymbolToken' is cid range [1715, 1716]
Type check for Type: class 'InheritedMember' is cid range [705, 706]
Type check for Type: class 'SourceLocation' is cid range [1579, 1579]
Type check for Type: class 'Directory' is cid range [2328, 2328]
Type check for Type: class 'BinaryOperationValue' is cid range [275, 277]
Type check for Type: class 'StringNode' is cid range [1308, 1310]
Type check for Type: class 'HBinaryBitOp' is cid range [1459, 1463]
Type check for Type: class 'Call' is cid range [483, 484]
Type check for Type: class 'ErroneousElementX' is cid range [2029, 2033]

[rmacnak, 2016-09-08 00:22:08]

flutter layout benchmark
816.6, 812.9, 819.8, 800.4, 820.6
=>
795.3, 796.5, 793.1, 788.0, 776.6
+3.05%

flutter build benchmark
1590.3, 1586.1, 1587.2, 1636.6, 1565.3
=>
1570.9, 1590.4, 1574.3, 1554.8, 1570.3
+1.37%

[rmacnak, 2016-09-08 01:02:16]

CompileOnceHelloHtml
1413, 1326, 1426, 1292, 1473
=>
1306, 1445, 1377, 1384, 1397
+0.33%

[siva, 2016-09-08 01:45:56]

asiva@google.com changed reviewers:
+ asiva@google.com

[siva, 2016-09-08 01:45:57]

Nice.

DBC

https://codereview.chromium.org/2314133003/diff/80001/runtime/vm/precompiler.cc
File runtime/vm/precompiler.cc (right):

https://codereview.chromium.org/2314133003/diff/80001/runtime/vm/precompiler....
runtime/vm/precompiler.cc:210: if (type.IsFunctionType()) return false;
Zone* zone = thread_->zone();

and use this zone in all the handles created in this method.

https://codereview.chromium.org/2314133003/diff/80001/runtime/vm/precompiler....
runtime/vm/precompiler.cc:275: type.ToCString(), *lower_limit, *upper_limit);
Left over debug print?

[rmacnak, 2016-09-08 21:02:46]

https://chromiumcodereview.appspot.com/2314133003/diff/80001/runtime/vm/preco...
File runtime/vm/precompiler.cc (right):

https://chromiumcodereview.appspot.com/2314133003/diff/80001/runtime/vm/preco...
runtime/vm/precompiler.cc:210: if (type.IsFunctionType()) return false;
On 2016/09/08 01:45:56, siva wrote:
> Zone* zone = thread_->zone();
> 
> and use this zone in all the handles created in this method.

Done.

https://chromiumcodereview.appspot.com/2314133003/diff/80001/runtime/vm/preco...
runtime/vm/precompiler.cc:275: type.ToCString(), *lower_limit, *upper_limit);
On 2016/09/08 01:45:56, siva wrote:
> Left over debug print?

Moved behind --trace-precompiler

[Florian Schneider, 2016-09-08 21:21:05]

Lgtm.

https://chromiumcodereview.appspot.com/2314133003/diff/100001/runtime/vm/aot_...
File runtime/vm/aot_optimizer.cc (right):

https://chromiumcodereview.appspot.com/2314133003/diff/100001/runtime/vm/aot_...
runtime/vm/aot_optimizer.cc:1571: String::Handle(Z, String::New(negate ?
"_classRangeCheckNegative"
Maybe add to symbols.h, and load from symbol table? Since these are identifiers
in the internal library, they will be in the symbol table anyway, right?

https://chromiumcodereview.appspot.com/2314133003/diff/100001/runtime/vm/aot_...
runtime/vm/aot_optimizer.cc:1572: : "_classRangeCheck"));
Same.

[rmacnak, 2016-09-08 22:20:21]

https://chromiumcodereview.appspot.com/2314133003/diff/100001/runtime/vm/aot_...
File runtime/vm/aot_optimizer.cc (right):

https://chromiumcodereview.appspot.com/2314133003/diff/100001/runtime/vm/aot_...
runtime/vm/aot_optimizer.cc:1571: String::Handle(Z, String::New(negate ?
"_classRangeCheckNegative"
On 2016/09/08 21:21:05, Florian Schneider wrote:
> Maybe add to symbols.h, and load from symbol table? Since these are
identifiers
> in the internal library, they will be in the symbol table anyway, right?

Done.

[rmacnak, 2016-09-08 22:25:13]

Description was changed from

==========
AOT: Use a cid range check when possible to implement type tests.
==========

to

==========
AOT: Use a cid range check when possible to implement type tests.

R=fschneider@google.com

Committed:
https://github.com/dart-lang/sdk/commit/6137d020b96a4d46de9de2cc357295d52a6a09f2
==========

[rmacnak, 2016-09-08 22:25:15]

Committed patchset #7 (id:120001) manually as
6137d020b96a4d46de9de2cc357295d52a6a09f2 (presubmit successful).