Use type info for the ToBoolean translation in crankshaft.

Use type info for the ToBoolean translation in crankshaft.

To do this, the Branch instruction needs to carry around a temporary register,
but only when the crankshafted code will make a map access. When the
crankshafted code sees an object of a type it hasn't encountered before, it will
always trigger a deopt. Another option in theses cases would be calling a
ToBooleanStub which can handle all types, but then one has to be careful to
*not* trigger a GC (which is currently a bit tricky to achieve).

Const-corrected ToBoolean::Types. Moved the NeedsMap logic into ToBoolean::Types
itself, where it belongs.

This patch improves a lot of benchmarks, crypto-orig even by 16.7%, but slows
down others. The slowdown has to be investigated, but I'd like to get this patch
out first to fix the flakiness problems we currently have due to the previous
crankshafted ToBoolean.
Committed: http://code.google.com/p/v8/source/detail?r=8758

[Sven Panne, 2011-07-28 12:50:38]

New version with 2 changes:

   a) Fixed a bug where string-only ToBoolean code ran into an incorrect deopt.
Moral of the story: Use EmitBranch only in tail positions. :-/

   b) When there is no type info for ToBoolean, generated code for handling all
cases instead of always-deopt-code. This is essential for some benchmarks like
checkers or deltablue. A probably better solution would be to collect type info
from other uses in the same function and use the union of those instead, but
this will be in a separate CL.

[danno, 2011-07-28 13:33:03]

lgtm

[fschneider, 2011-07-28 13:54:58]

LGTM as well.

http://codereview.chromium.org/7461107/diff/9001/src/ia32/lithium-codegen-ia3...
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/7461107/diff/9001/src/ia32/lithium-codegen-ia3...
src/ia32/lithium-codegen-ia32.cc:1395: Factory* factory = this->factory();
Doesn't the compiler inline the factory() accessor?

http://codereview.chromium.org/7461107/diff/9001/src/ia32/lithium-codegen-ia3...
src/ia32/lithium-codegen-ia32.cc:1406: if (expected.IsEmpty()) expected =
ToBooleanStub::all_types();
We should watch out for code size a little. If this case happens often, we will
generate lots of inline code. Can you compare performance when calling the stub
in this case?

http://codereview.chromium.org/7461107/diff/9001/src/ia32/lithium-codegen-ia3...
src/ia32/lithium-codegen-ia32.cc:1423: // We've seen a string for the first time
-> deopt.
Update comment: s/string/boolean

http://codereview.chromium.org/7461107/diff/9001/src/ia32/lithium-codegen-ia3...
src/ia32/lithium-codegen-ia32.cc:1433: // We've seen a string for the first time
-> deopt.
Same here.

http://codereview.chromium.org/7461107/diff/9001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

http://codereview.chromium.org/7461107/diff/9001/src/ia32/lithium-ia32.cc#new...
src/ia32/lithium-ia32.cc:1048: return AssignEnvironment(branch);
If we cover all cases, then we don't deopt, right? In this case you can avoid
assigning an environment to reduce the number of live values.

[Sven Panne, 2011-07-28 14:12:44]

I'll upload a tiny CL for this stuff now...

http://codereview.chromium.org/7461107/diff/9001/src/ia32/lithium-codegen-ia3...
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/7461107/diff/9001/src/ia32/lithium-codegen-ia3...
src/ia32/lithium-codegen-ia32.cc:1395: Factory* factory = this->factory();
On 2011/07/28 13:54:58, fschneider wrote:
> Doesn't the compiler inline the factory() accessor?

Ooops, I haven't seen that retrieving the current isolate is cheap here, because
we can get it via an instance field. So this is local variable is probably not
needed, I'll remove it.

http://codereview.chromium.org/7461107/diff/9001/src/ia32/lithium-codegen-ia3...
src/ia32/lithium-codegen-ia32.cc:1406: if (expected.IsEmpty()) expected =
ToBooleanStub::all_types();
On 2011/07/28 13:54:58, fschneider wrote:
> We should watch out for code size a little. If this case happens often, we
will
> generate lots of inline code. Can you compare performance when calling the
stub
> in this case?

In the programs I've seen so far this doesn't happen very often, but when it
happens, it happens in "hot" code a lot, so I would really like to avoid the
overhead of calling a stub.

What would really help regarding code size and speed would be a different
handling of undetectable objects, but that is a different story...

http://codereview.chromium.org/7461107/diff/9001/src/ia32/lithium-codegen-ia3...
src/ia32/lithium-codegen-ia32.cc:1423: // We've seen a string for the first time
-> deopt.
On 2011/07/28 13:54:58, fschneider wrote:
> Update comment: s/string/boolean

Done.

http://codereview.chromium.org/7461107/diff/9001/src/ia32/lithium-codegen-ia3...
src/ia32/lithium-codegen-ia32.cc:1433: // We've seen a string for the first time
-> deopt.
On 2011/07/28 13:54:58, fschneider wrote:
> Same here.

Done.