Recognize special comparisons via pattern matching on the hydrogen graph.

Recognize special comparisons via pattern matching on the hydrogen graph.

Previously, comparisons against null/undefined and comparisons of the result of
typeof against a constant string were done syntactically. Now we do this via
pattern matching on the hydrogen graph, which opens up more opportunities for
better code generation, e.g. the following comparisons are now recognized to be
special:

   var bar = typeof foo;
   var baz = "undefined";
   if (bar == baz) ...

   var blah = undefined;
   if (hurz == blah) ...

If we did this handling of special cases even later at lithium generation time,
even more cases could be recognized, but this would involve bigger changes and
this CL handles most common cases.
Committed: http://code.google.com/p/v8/source/detail?r=9612

[Kevin Millikin (Chromium), 2011-10-13 13:11:59]

Much better to do this at graph construction.  Comments below.

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

http://codereview.chromium.org/8242002/diff/1/src/hydrogen.cc#newcode5793
src/hydrogen.cc:5793: void
HGraphBuilder::HandleLiteralCompareTypeof(CompareOperation* expr,
I'd prefer to use more specific types here.  The casts below will always succeed
in this change, but it needs nonlocal reasoning to see that (and relies on the
assertion to preserve it in the face of changes to the helper functions).

void HGraphBuilder::HandleLiteralCompareTypeof(CompareOperation* expr,
                                               HTypeof* typeof_expr,
                                               Handle<String> blah) { ...

http://codereview.chromium.org/8242002/diff/1/src/hydrogen.cc#newcode5797
src/hydrogen.cc:5797: Handle<String> blah =
Handle<String>::cast(HConstant::cast(check)->handle());
There must be a better name.

http://codereview.chromium.org/8242002/diff/1/src/hydrogen.cc#newcode5801
src/hydrogen.cc:5801: typeof_expr->DeleteAndReplaceWith(instr);
I don't think this is right.  instr is a control instruction, and I think we're
relying on having no uses for those.  It should be the case that there is no use
of typeof_expr, so you can replace it with NULL.

An alternative that I like a bit better, because it covers more cases, is to
make HTypeof::Canonicalize() return NULL if there are no uses (provided we
convince ourselves that typeof cannot have side effects).

http://codereview.chromium.org/8242002/diff/1/src/hydrogen.cc#newcode5829
src/hydrogen.cc:5829: *typeof_expr = left;
If you take my suggestion above, this is where you'd cast:

*typeof_expr = HTypeof::cast(left);
*check = Handle<String>::cast(HConstant::cast(right)->handle());

http://codereview.chromium.org/8242002/diff/1/src/hydrogen.cc#newcode5891
src/hydrogen.cc:5891: HValue* typeof_expr = NULL;
And

HTypeof* typeof_expr = NULL;
Handle<String> check;

[Sven Panne, 2011-10-13 14:21:38]

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

http://codereview.chromium.org/8242002/diff/1/src/hydrogen.cc#newcode5793
src/hydrogen.cc:5793: void
HGraphBuilder::HandleLiteralCompareTypeof(CompareOperation* expr,
On 2011/10/13 13:11:59, Kevin Millikin wrote:
> I'd prefer to use more specific types here.  The casts below will always
succeed
> in this change, but it needs nonlocal reasoning to see that (and relies on the
> assertion to preserve it in the face of changes to the helper functions).
> 
> void HGraphBuilder::HandleLiteralCompareTypeof(CompareOperation* expr,
>                                                HTypeof* typeof_expr,
>                                                Handle<String> blah) { ...

Done.

http://codereview.chromium.org/8242002/diff/1/src/hydrogen.cc#newcode5797
src/hydrogen.cc:5797: Handle<String> blah =
Handle<String>::cast(HConstant::cast(check)->handle());
On 2011/10/13 13:11:59, Kevin Millikin wrote:
> There must be a better name.

hurz? (http://www.youtube.com/watch?v=RAx0P-8n5K4)
gnlpfth? (http://www.youtube.com/watch?v=olGad2Uhnwc) ;-)
Luckily, we don't need this variable anymore.

http://codereview.chromium.org/8242002/diff/1/src/hydrogen.cc#newcode5801
src/hydrogen.cc:5801: typeof_expr->DeleteAndReplaceWith(instr);
On 2011/10/13 13:11:59, Kevin Millikin wrote:
> I don't think this is right.  instr is a control instruction, and I think
we're
> relying on having no uses for those.  It should be the case that there is no
use
> of typeof_expr, so you can replace it with NULL.
> 
> An alternative that I like a bit better, because it covers more cases, is to
> make HTypeof::Canonicalize() return NULL if there are no uses (provided we
> convince ourselves that typeof cannot have side effects).

I'd like to go for the first alternative (replacing with NULL) for now, because
it is a more local change of the CL. I'll have a look at the Canonicalize()
alternative later and submit a different CL if it makes things clearer.

http://codereview.chromium.org/8242002/diff/1/src/hydrogen.cc#newcode5829
src/hydrogen.cc:5829: *typeof_expr = left;
On 2011/10/13 13:11:59, Kevin Millikin wrote:
> If you take my suggestion above, this is where you'd cast:
> 
> *typeof_expr = HTypeof::cast(left);
> *check = Handle<String>::cast(HConstant::cast(right)->handle());

Done.

http://codereview.chromium.org/8242002/diff/1/src/hydrogen.cc#newcode5891
src/hydrogen.cc:5891: HValue* typeof_expr = NULL;
On 2011/10/13 13:11:59, Kevin Millikin wrote:
> And
> 
> HTypeof* typeof_expr = NULL;
> Handle<String> check;

Done.

[Kevin Millikin (Chromium), 2011-10-13 14:38:41]

lgtm