Fix to_boolean type feedback for unary and binary ops

Fix to_boolean type feedback for unary and binary ops

BUG=
R=jkummerow@chromium.org, rossberg@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=15319

[rossberg, 2013-06-24 08:54:16]

LGTM with comment

https://codereview.chromium.org/17444011/diff/1/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/17444011/diff/1/src/ast.h#newcode405
src/ast.h:405: 
Nit: I'd remove that empty line

https://codereview.chromium.org/17444011/diff/1/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/17444011/diff/1/src/ia32/lithium-codegen-ia32...
src/ia32/lithium-codegen-ia32.cc:2243: if (!info()->IsStub()) {
This seems redundant, given that DeoptimizeIf already makes the distinction.
(Same for other platforms)

[Jakob Kummerow, 2013-06-24 09:25:07]

For the record, as discussed offline, this change does not LGTM in its current
form, as it breaks the existing logic. Maybe what we need instead is separate
to-boolean type feedback for expr->right?

(I also have a few drive-by nits.)

https://codereview.chromium.org/17444011/diff/1/src/ast.cc
File src/ast.cc (right):

https://codereview.chromium.org/17444011/diff/1/src/ast.cc#newcode436
src/ast.cc:436: TypeFeedbackId origin) {
nit: align arguments with each other

https://codereview.chromium.org/17444011/diff/1/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/17444011/diff/1/src/ast.h#newcode388
src/ast.h:388: TypeFeedbackOracle* oracle, TypeFeedbackId origin);
nit: one line per argument

https://codereview.chromium.org/17444011/diff/1/src/typing.cc
File src/typing.cc (right):

https://codereview.chromium.org/17444011/diff/1/src/typing.cc#newcode444
src/typing.cc:444: expr->RecordToBooleanTypeFeedback(oracle(),
expr->left()->test_id());
I don't think this change is correct, seeing that in hydrogen.cc:9584 the type
feedback is read specifically from expr->left:

ToBooleanStub::Types expected(expr->left()->to_boolean_types());

[oliv, 2013-06-24 13:38:25]

On 2013/06/24 09:25:07, Jakob wrote:
> For the record, as discussed offline, this change does not LGTM in its current
> form, as it breaks the existing logic. Maybe what we need instead is separate
> to-boolean type feedback for expr->right?
> 
> (I also have a few drive-by nits.)
> 
> https://codereview.chromium.org/17444011/diff/1/src/ast.cc
> File src/ast.cc (right):
> 
> https://codereview.chromium.org/17444011/diff/1/src/ast.cc#newcode436
> src/ast.cc:436: TypeFeedbackId origin) {
> nit: align arguments with each other
> 
> https://codereview.chromium.org/17444011/diff/1/src/ast.h
> File src/ast.h (right):
> 
> https://codereview.chromium.org/17444011/diff/1/src/ast.h#newcode388
> src/ast.h:388: TypeFeedbackOracle* oracle, TypeFeedbackId origin);
> nit: one line per argument
> 
> https://codereview.chromium.org/17444011/diff/1/src/typing.cc
> File src/typing.cc (right):
> 
> https://codereview.chromium.org/17444011/diff/1/src/typing.cc#newcode444
> src/typing.cc:444: expr->RecordToBooleanTypeFeedback(oracle(),
> expr->left()->test_id());
> I don't think this change is correct, seeing that in hydrogen.cc:9584 the type
> feedback is read specifically from expr->left:
> 
> ToBooleanStub::Types expected(expr->left()->to_boolean_types());

Thanks for pointing this out. Indeed the type info in VisitLogicalExpression is
collected from the left hand operand. But of course the TypeInfo in
TestContext::Return comes from the testContext and not the right hand operand.
This version is therefore a bit more precise, so PTAL

[Jakob Kummerow, 2013-06-24 15:06:35]

LGTM with nits.

https://codereview.chromium.org/17444011/diff/15001/src/ast.cc
File src/ast.cc (right):

https://codereview.chromium.org/17444011/diff/15001/src/ast.cc#newcode292
src/ast.cc:292: // TODO(oli) If this Operation is used in a test context, then
the expression
nit: the identifier in the parentheses should be the @chromium.org username, so
"olivf" in your case.

https://codereview.chromium.org/17444011/diff/15001/src/ast.cc#newcode293
src/ast.cc:293: // has a ToBool stub and we want to collect this information.
However the
nit: s/ToBool/ToBoolean/

https://codereview.chromium.org/17444011/diff/15001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/17444011/diff/15001/src/ast.h#newcode398
src/ast.h:398: byte to_boolean_types_;
nit: the style guide forbids non-private fields. Use a protected setter, leave
the field itself private.

[oliv, 2013-06-25 11:49:52]

Committed patchset #3 manually as r15319 (presubmit successful).