Issue 10826285: Optimize equality for case when all targets are Object.equals.

Issue 10826285: Optimize equality for case when all targets are Object.equals. (Closed)

Created:
8 years, 4 months ago by srdjan

Modified:
8 years, 4 months ago

Reviewers:
regis, Florian Schneider

CC:
reviews_dartlang.org, vm-dev_dartlang.org

Base URL:
http://dart.googlecode.com/svn/branches/bleeding_edge/dart/

Visibility:
Public.

More Reviews

Description

Optimize equality for case when all targets are Object.equals. Also fixes a couple of issues with https://chromiumcodereview.appspot.com/10830275/ Committed: https://code.google.com/p/dart/source/detail?r=10615

Patch Set 8 : #

Total comments: 6

Created: 8 years, 4 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+232 lines, -30 lines)			Patch
M	runtime/vm/flow_graph_optimizer.cc	View	1 2 3 4 5 6 7	2 chunks	+8 lines, -1 line	0 comments	Download
M	runtime/vm/intermediate_language.h	View	1 2 3 4 5 6 7	3 chunks	+4 lines, -2 lines	0 comments	Download
M	runtime/vm/intermediate_language.cc	View	1 2 3 4 5 6 7	2 chunks	+24 lines, -14 lines	6 comments	Download
M	runtime/vm/intermediate_language_ia32.cc	View	1 2 3 4 5 6 7	9 chunks	+82 lines, -7 lines	0 comments	Download
M	runtime/vm/intermediate_language_x64.cc	View	1 2 3 4 5 6 7	8 chunks	+81 lines, -6 lines	0 comments	Download
M	runtime/vm/object.h	View	1 2 3 4 5 6 7	1 chunk	+3 lines, -0 lines	0 comments	Download
M	runtime/vm/object.cc	View	1 2 3 4 5 6 7	1 chunk	+30 lines, -0 lines	0 comments	Download

Messages

Total messages: 6 (0 generated)

Expand Messages | Collapse Messages

regis

LGTM https://chromiumcodereview.appspot.com/10826285/diff/7009/runtime/vm/intermediate_language.cc File runtime/vm/intermediate_language.cc (right): https://chromiumcodereview.appspot.com/10826285/diff/7009/runtime/vm/intermediate_language.cc#newcode1059 runtime/vm/intermediate_language.cc:1059: left_type.IsSubtypeOf(Type::Handle(Type::BoolInterface()), NULL)) { You should be using Value::CompileTypeIsMoreSpecificThan ...

8 years, 4 months ago (2012-08-13 23:06:19 UTC) #2

Florian Schneider

dbc https://chromiumcodereview.appspot.com/10826285/diff/13/runtime/vm/intermediate_language.cc File runtime/vm/intermediate_language.cc (right): https://chromiumcodereview.appspot.com/10826285/diff/13/runtime/vm/intermediate_language.cc#newcode1045 runtime/vm/intermediate_language.cc:1045: return NULL; Accidental edit? I don't see why ...

8 years, 4 months ago (2012-08-15 11:29:57 UTC) #3

srdjan

https://chromiumcodereview.appspot.com/10826285/diff/7009/runtime/vm/intermediate_language.cc File runtime/vm/intermediate_language.cc (right): https://chromiumcodereview.appspot.com/10826285/diff/7009/runtime/vm/intermediate_language.cc#newcode1059 runtime/vm/intermediate_language.cc:1059: left_type.IsSubtypeOf(Type::Handle(Type::BoolInterface()), NULL)) { On 2012/08/13 23:06:19, regis wrote: > ...

8 years, 4 months ago (2012-08-15 13:54:24 UTC) #4

Florian Schneider

https://chromiumcodereview.appspot.com/10826285/diff/13/runtime/vm/intermediate_language.cc File runtime/vm/intermediate_language.cc (right): https://chromiumcodereview.appspot.com/10826285/diff/13/runtime/vm/intermediate_language.cc#newcode1045 runtime/vm/intermediate_language.cc:1045: return NULL; On 2012/08/15 13:54:25, srdjan wrote: > On ...

8 years, 4 months ago (2012-08-15 14:15:56 UTC) #5

srdjan

8 years, 4 months ago (2012-08-15 15:36:06 UTC) #6

https://chromiumcodereview.appspot.com/10826285/diff/13/runtime/vm/intermedia...
File runtime/vm/intermediate_language.cc (right):

https://chromiumcodereview.appspot.com/10826285/diff/13/runtime/vm/intermedia...
runtime/vm/intermediate_language.cc:1045: return NULL;
On 2012/08/15 14:15:56, Florian Schneider wrote:
> On 2012/08/15 13:54:25, srdjan wrote:
> > On 2012/08/15 11:29:57, Florian Schneider wrote:
> > > Accidental edit? I don't see why this should be disabled. 
> > 
> > Yes, accidental edit, but correct. See other email, we cannot rely on
> > CompileType at the moment.
> 
> I'm not convinced: This optimization relies only on the fact that the left
> expression is guaranteed to be a boolean.
> 
> The only expressions that return a CompileType() of BoolInterface are:
> 
> AssertBoolean
> StrictCompare
> EqualityCompare, RelationalOp iff operands cid are {smi, double, number}
> BooleanNegate

If the left expression's CompileType says that it is Boollnterface, it may be
null-class as well (OK for assignability, not OK for removing class checks).

Example:


foo(a) {
  var x;
  if (a > 0) {
    // x = a == 1;  // x's compile-type will be bool interface.
    x = true;  // x's compile-type will be Bool type.
  } else {
    x = null;
  }
  return x === true;
}

https://chromiumcodereview.appspot.com/10826285/diff/13/runtime/vm/intermedia...
runtime/vm/intermediate_language.cc:1059:
left_use->CompileTypeIsMoreSpecificThan(
On 2012/08/15 14:15:56, Florian Schneider wrote:
> Not sure if this works with null: Which other type that is more specific than
> Bool would be valid here?
> 
> I think the optimization is valid if the CompileType is _exactly_ Bool.

The only type more specific than bool interface is Bool class.

Expand Messages | Collapse Messages