Dart VM: Simplify code generation for equality operators.

Dart VM: Simplify code generation for equality operators.

By inserting the necessary checks for null inside the callee
at the AST level, the code generation of == operations can be
greatly simplified.

This is a performance-neutral change and a step for allowing
generic inlining of arbitrary == methods. So far we could only
inline them for a common set of types in the flow graph
optimizer.

R=srdjan@google.com

Committed: https://code.google.com/p/dart/source/detail?r=28084

[hausner, 2013-09-23 16:51:06]

This breaks debugging, in particular setting breakpoints on == statements.

Also, have Srdjan review this since it also changes how the VM collects type
feedback.

https://codereview.chromium.org/24203004/diff/12001/runtime/vm/stub_code_ia32.cc
File runtime/vm/stub_code_ia32.cc (left):

https://codereview.chromium.org/24203004/diff/12001/runtime/vm/stub_code_ia32...
runtime/vm/stub_code_ia32.cc:2043: void
StubCode::GenerateEqualityWithNullArgStub(Assembler* assembler) {
As best as I can tell removing this stub breaks debugging, and doesn't let the
VM collect IC data on arguments.

If you inline the null checks on the AST level, the debugger has no chance to
reach a safe point when one of the arguments is null, since no runtime call and
no method call ever takes place.

[Florian Schneider, 2013-09-23 18:02:36]

I'll take a look at impact on debugging and add a debugger test if there is not
already one.

https://codereview.chromium.org/24203004/diff/12001/runtime/vm/stub_code_ia32.cc
File runtime/vm/stub_code_ia32.cc (left):

https://codereview.chromium.org/24203004/diff/12001/runtime/vm/stub_code_ia32...
runtime/vm/stub_code_ia32.cc:2043: void
StubCode::GenerateEqualityWithNullArgStub(Assembler* assembler) {
On 2013/09/23 16:51:06, hausner wrote:
> As best as I can tell removing this stub breaks debugging, and doesn't let the
> VM collect IC data on arguments.
> 
> If you inline the null checks on the AST level, the debugger has no chance to
> reach a safe point when one of the arguments is null, since no runtime call
and
> no method call ever takes place.

I don't see how: == is still a plain method call. It is the callee that is
modified to check at method entry if the other argument is null.

Type feedback is collected like for normal method calls.

All current debugger tests pass. Is there a debugger test that specifically
tests breaking at ==?

[Florian Schneider, 2013-09-25 15:54:46]

I added a debugger test: Breakpoints work as before. Single stepping needed a
small change to skip the additional AST nodes that I'm inserting at the start of
every == method. They get a token position of 0 (kDummyTokenIndex) and the
debugger is changed to ignure calls with this token position when single
stepping.

[hausner, 2013-09-25 16:36:10]

Thanks for adding the test. See comment about additional test case when
comparing directly with the null constant.

https://codereview.chromium.org/24203004/diff/24001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/24203004/diff/24001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:2854: ParseNativeFunctionBlock(&params, func);
This is a nit, but your scheme does not work for native implementations of ==. I
am fine with that. Native implementations just have to do the null check
themselves.

https://codereview.chromium.org/24203004/diff/24001/tests/standalone/debugger...
File tests/standalone/debugger/step_in_equals_test.dart (right):

https://codereview.chromium.org/24203004/diff/24001/tests/standalone/debugger...
tests/standalone/debugger/step_in_equals_test.dart:18: var x = a == b;  //
Breakpoint #1.
I think the case of comparing to null explicitly is handled through a different
code path in the compiler. Please add a test similar to:

if (x == null) {  // make sure you can break on this line.
  ...
}

[Florian Schneider, 2013-09-26 11:34:21]

https://codereview.chromium.org/24203004/diff/24001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/24203004/diff/24001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:2854: ParseNativeFunctionBlock(&params, func);
On 2013/09/25 16:36:11, hausner wrote:
> This is a nit, but your scheme does not work for native implementations of ==.
I
> am fine with that. Native implementations just have to do the null check
> themselves. 

Good point. Right now, native methods contain just the NativeCall node. To be
consistent, I'll add the check to the AST  of the native method body, so that
the native implementations are never called with null arguments.

https://codereview.chromium.org/24203004/diff/24001/tests/standalone/debugger...
File tests/standalone/debugger/step_in_equals_test.dart (right):

https://codereview.chromium.org/24203004/diff/24001/tests/standalone/debugger...
tests/standalone/debugger/step_in_equals_test.dart:18: var x = a == b;  //
Breakpoint #1.
On 2013/09/25 16:36:11, hausner wrote:
> I think the case of comparing to null explicitly is handled through a
different
> code path in the compiler. Please add a test similar to:
> 
> if (x == null) {  // make sure you can break on this line.
>   ...
> }

Done. Appended (x == null) and (null == x) to the test.

[srdjan, 2013-09-26 19:49:15]

DBC:
Nice cleanup! Please run a performance check, just in case.

[Florian Schneider, 2013-09-30 09:30:05]

On 2013/09/26 19:49:15, srdjan wrote:
> DBC:
> Nice cleanup! Please run a performance check, just in case.

Yes, I did a performance measurement: Execution time in not affected
significantly, code size is slightly better.

Any more comments, or does it look good?

[srdjan, 2013-09-30 14:36:20]

On 2013/09/30 09:30:05, Florian Schneider wrote:
> On 2013/09/26 19:49:15, srdjan wrote:
> > DBC:
> > Nice cleanup! Please run a performance check, just in case.
> 
> Yes, I did a performance measurement: Execution time in not affected
> significantly, code size is slightly better.
> 
> Any more comments, or does it look good?

No more comments, LGTM

[Florian Schneider, 2013-10-01 10:11:29]

Committed patchset #5 manually as r28084 (presubmit successful).