Migrate BinaryOpICs and UnaryOpICs to new type rep

Migrate BinaryOpICs and UnaryOpICs to new type rep

R=rossberg@chromium.org

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

[Jakob Kummerow, 2013-06-13 11:28:46]

Please take a look.

[rossberg, 2013-06-13 14:24:30]

https://codereview.chromium.org/16957004/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/16957004/diff/1/src/hydrogen.cc#newcode9458
src/hydrogen.cc:9458: ASSERT(right_type->Is(Type::None()));
This might no longer be true in the future. I think the condition needs to
change to

  if (left->Is(None) || right->Is(None))

And the defaulting to Any below refined accordingly.

https://codereview.chromium.org/16957004/diff/1/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/16957004/diff/1/src/ic.cc#newcode2513
src/ic.cc:2513: case SMI:           return handle(Type::Integer31(), isolate);
Nit: can we get rid of the non-standard layout here?

https://codereview.chromium.org/16957004/diff/1/src/ic.cc#newcode2517
src/ic.cc:2517: return handle(Type::Optional(handle(Type::Number(), isolate)),
isolate);
That's a change, is that correct? In particular, why Number and not String?

[Jakob Kummerow, 2013-06-14 14:26:56]

https://codereview.chromium.org/16957004/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/16957004/diff/1/src/hydrogen.cc#newcode9458
src/hydrogen.cc:9458: ASSERT(right_type->Is(Type::None()));
On 2013/06/13 14:24:30, rossberg wrote:
> This might no longer be true in the future. I think the condition needs to
> change to
> 
>   if (left->Is(None) || right->Is(None))
> 
> And the defaulting to Any below refined accordingly.

Done.

https://codereview.chromium.org/16957004/diff/1/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/16957004/diff/1/src/ic.cc#newcode2513
src/ic.cc:2513: case SMI:           return handle(Type::Integer31(), isolate);
On 2013/06/13 14:24:30, rossberg wrote:
> Nit: can we get rid of the non-standard layout here?

Done -- although I kind of liked it the way it was.

https://codereview.chromium.org/16957004/diff/1/src/ic.cc#newcode2517
src/ic.cc:2517: return handle(Type::Optional(handle(Type::Number(), isolate)),
isolate);
On 2013/06/13 14:24:30, rossberg wrote:
> That's a change, is that correct? In particular, why Number and not String?

Oops, overlooked that the stub handles strings too. Good catch. Fixed.

[rossberg, 2013-06-14 14:45:45]

LGTM with comment

https://codereview.chromium.org/16957004/diff/5001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/16957004/diff/5001/src/hydrogen.cc#newcode9461
src/hydrogen.cc:9461: AddSoftDeoptimize();
Hm, this version will produce two deopts in the common case. Not wrong, but a
bit ugly.

[Jakob Kummerow, 2013-06-14 14:47:27]

https://codereview.chromium.org/16957004/diff/5001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/16957004/diff/5001/src/hydrogen.cc#newcode9461
src/hydrogen.cc:9461: AddSoftDeoptimize();
On 2013/06/14 14:45:45, rossberg wrote:
> Hm, this version will produce two deopts in the common case. Not wrong, but a
> bit ugly.

No, it won't. If the current block is already marked as having a soft deopt,
AddSoftDeoptimize() returns without doing anything.

[Jakob Kummerow, 2013-06-14 17:02:48]

Committed patchset #2 manually as r15172 (presubmit successful).