Better codegen for '<expression> === void <literal>'.

Better codegen for '<expression> === void <literal>'.

Detect the pattern in both, the full compiler and crankshaft and generate direct pointer
comparisons. Along the way I cleaned up 'typeof <expression> == <string literal>' comparisons
as well by lifting platform independent code and checking the symmetric case.

BUG=v8:1440
TEST=cctest/test-api.cc

Committed: http://code.google.com/p/v8/source/detail?r=8420

[Steven, 2011-06-21 10:01:28]

Hi Lasse.
As discussed over VC you wanted to review this. PTAL.
Thanks!
--
Steven

[Lasse Reichstein, 2011-06-21 10:12:45]

Hi there.
Actually, I was asking for changes in the Parser, not in codegen. I'll forward
this to fschneider.

[fschneider, 2011-06-21 11:36:08]

http://codereview.chromium.org/7216008/diff/1/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):

http://codereview.chromium.org/7216008/diff/1/src/arm/full-codegen-arm.cc#new...
src/arm/full-codegen-arm.cc:4034: { AccumulatorValueContext context(this);
This expression should not be visited in a typeof-context. Instead replace the
whole block with just

VisitForAccumulatorValue(expr)

The difference is in a typeof-expression there is no ReferenceError thrown if
the sub-expression is not defined.
Maybe the unit test should be extended to catch that case.

http://codereview.chromium.org/7216008/diff/1/src/full-codegen.cc
File src/full-codegen.cc (right):

http://codereview.chromium.org/7216008/diff/1/src/full-codegen.cc#newcode1323
src/full-codegen.cc:1323: 
Remove extra line.

http://codereview.chromium.org/7216008/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/7216008/diff/1/src/hydrogen.cc#newcode5550
src/hydrogen.cc:5550: // Check for the pattern: typeof <expression> == <string
literal>.
Since this pattern matching code is exactly the same in the two code generators,
it would be a good idea to share it to avoid code duplication. You could make a
predicate on the AST node CompareOperation in ast.h/ast.cc and write sth like:

if (expr->IsLiteralCompareTypeof() {
  return HandleLiteralCompareTypeof(expr)
}

if  (expr->IsLiteralCompareUndefined()) {
  return HandleLiteralCompareUndefined(expr);
}

http://codereview.chromium.org/7216008/diff/1/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

http://codereview.chromium.org/7216008/diff/1/src/ia32/full-codegen-ia32.cc#n...
src/ia32/full-codegen-ia32.cc:3996: { AccumulatorValueContext context(this);
Same comment as on ARM.

http://codereview.chromium.org/7216008/diff/1/src/x64/full-codegen-x64.cc
File src/x64/full-codegen-x64.cc (right):

http://codereview.chromium.org/7216008/diff/1/src/x64/full-codegen-x64.cc#new...
src/x64/full-codegen-x64.cc:3974: { AccumulatorValueContext context(this);
Same comment as on the ARM file.

[Steven, 2011-06-22 10:00:53]

http://codereview.chromium.org/7216008/diff/1/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):

http://codereview.chromium.org/7216008/diff/1/src/arm/full-codegen-arm.cc#new...
src/arm/full-codegen-arm.cc:4034: { AccumulatorValueContext context(this);
On 2011/06/21 11:36:08, fschneider wrote:
> This expression should not be visited in a typeof-context. Instead replace the
> whole block with just
> 
> VisitForAccumulatorValue(expr)
> 
> The difference is in a typeof-expression there is no ReferenceError thrown if
> the sub-expression is not defined.
> Maybe the unit test should be extended to catch that case. 

Done.

http://codereview.chromium.org/7216008/diff/1/src/full-codegen.cc
File src/full-codegen.cc (right):

http://codereview.chromium.org/7216008/diff/1/src/full-codegen.cc#newcode1323
src/full-codegen.cc:1323: 
On 2011/06/21 11:36:08, fschneider wrote:
> Remove extra line.

Done.

http://codereview.chromium.org/7216008/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/7216008/diff/1/src/hydrogen.cc#newcode5550
src/hydrogen.cc:5550: // Check for the pattern: typeof <expression> == <string
literal>.
On 2011/06/21 11:36:08, fschneider wrote:
> Since this pattern matching code is exactly the same in the two code
generators,
> it would be a good idea to share it to avoid code duplication. You could make
a
> predicate on the AST node CompareOperation in ast.h/ast.cc and write sth like:
> 
> if (expr->IsLiteralCompareTypeof() {
>   return HandleLiteralCompareTypeof(expr)
> }
> 
> if  (expr->IsLiteralCompareUndefined()) {
>   return HandleLiteralCompareUndefined(expr);
> }

Done.

http://codereview.chromium.org/7216008/diff/1/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

http://codereview.chromium.org/7216008/diff/1/src/ia32/full-codegen-ia32.cc#n...
src/ia32/full-codegen-ia32.cc:3996: { AccumulatorValueContext context(this);
On 2011/06/21 11:36:08, fschneider wrote:
> Same comment as on ARM.

Done.

http://codereview.chromium.org/7216008/diff/1/src/x64/full-codegen-x64.cc
File src/x64/full-codegen-x64.cc (right):

http://codereview.chromium.org/7216008/diff/1/src/x64/full-codegen-x64.cc#new...
src/x64/full-codegen-x64.cc:3974: { AccumulatorValueContext context(this);
On 2011/06/21 11:36:08, fschneider wrote:
> Same comment as on the ARM file.

Done.

[Steven, 2011-06-22 10:04:43]

I did a rebase in between to get the merger of HCompareJsObjectEq and
HCompareSymbolEq. So not all differences between patch set 1 and patch set 2 are
from me.

[fschneider, 2011-06-24 12:04:42]

LGTM.

http://codereview.chromium.org/7216008/diff/4001/src/ast.cc
File src/ast.cc (right):

http://codereview.chromium.org/7216008/diff/4001/src/ast.cc#newcode369
src/ast.cc:369: bool CompareOperation::IsLiteralCompareUndefined(Expression
**expr) {
For consistency:

Expression** expr

[Steven, 2011-06-24 14:33:29]

http://codereview.chromium.org/7216008/diff/4001/src/ast.cc
File src/ast.cc (right):

http://codereview.chromium.org/7216008/diff/4001/src/ast.cc#newcode369
src/ast.cc:369: bool CompareOperation::IsLiteralCompareUndefined(Expression
**expr) {
On 2011/06/24 12:04:42, fschneider wrote:
> For consistency:
> 
> Expression** expr

Done.