Add monomorphic CompareNilICs and Crankshaft support

Add monomorphic CompareNilICs and Crankshaft support

R=mvstanton@chromium.org

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

[mvstanton, 2013-04-23 09:27:30]

Cool stuff! Just a few comments.

https://codereview.chromium.org/14367018/diff/17001/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/14367018/diff/17001/src/code-stubs.cc#newcode434
src/code-stubs.cc:434: if (object->IsUndetectableObject() ||
!object->IsHeapObject()) {
Could you combine the if with the else above

https://codereview.chromium.org/14367018/diff/17001/src/code-stubs.cc#newcode437
src/code-stubs.cc:437: if ((types &
CompareNilICStub::kCompareAgainstMonomorphicMap) != 0) {
And again here.

https://codereview.chromium.org/14367018/diff/17001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/14367018/diff/17001/src/hydrogen.cc#newcode906
src/hydrogen.cc:906: ASSERT(!(did_then_ ^ did_else_));
I'd probably comment this in english: "Make sure we handled both then and else,
or neither." This took me a minute to figure out.

https://codereview.chromium.org/14367018/diff/17001/src/hydrogen.cc#newcode1806
src/hydrogen.cc:1806: BuildCheckMap(value, map);
As I understand it, this BuildCheckMap is important because it embeds the map as
the first map in reloc handles for the IC. The function is used in 2 ways...in
the inlined case, there is never a replacement done on the map. In the IC case,
then this map gets replaced when copying from a template.

I could imagine a BuildCompareNilScope or factory method that could ensure on
teardown that the first handle is the "sentinel map" passed in above. But that
is probably overkill. Or could an assert be placed right before BuildCheckMap
that somehow makes sure there are *no* reloc handles in the code object to this
point? (only in IC case)

In lieu of that I'd highlight the BuildCheckMap with a comment, and also comment
the declaration in hydrogen.h for this special, interesting concern.

https://codereview.chromium.org/14367018/diff/17001/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/14367018/diff/17001/src/ic.cc#newcode2806
src/ic.cc:2806: if (Smi::FromInt(object->IsNull() || object->IsUndefined())) {
Do you intend to have Smi::FromInt(... in the if statement? Something seems
wrong.

https://codereview.chromium.org/14367018/diff/17001/src/lithium.cc
File src/lithium.cc (right):

https://codereview.chromium.org/14367018/diff/17001/src/lithium.cc#newcode332
src/lithium.cc:332: if (block->predecessors()->length() > 1) can_eliminate =
false;
What is it unique about this CL that requires this change? Is it use of blocks
for push/pop of environment HValues?

https://codereview.chromium.org/14367018/diff/17001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/14367018/diff/17001/src/objects.h#newcode4783
src/objects.h:4783: STATIC_ASSERT(ExtraICStateField::kShift ==
ExtendedExtraICStateField::kShift);
This is getting pretty hairy. I'm glad for the static asserts!

[danno, 2013-04-23 16:14:47]

https://codereview.chromium.org/14367018/diff/17001/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/14367018/diff/17001/src/code-stubs.cc#newcode434
src/code-stubs.cc:434: if (object->IsUndetectableObject() ||
!object->IsHeapObject()) {
On 2013/04/23 09:27:30, mvstanton wrote:
> Could you combine the if with the else above

Done.

https://codereview.chromium.org/14367018/diff/17001/src/code-stubs.cc#newcode437
src/code-stubs.cc:437: if ((types &
CompareNilICStub::kCompareAgainstMonomorphicMap) != 0) {
On 2013/04/23 09:27:30, mvstanton wrote:
> And again here.

Done.

https://codereview.chromium.org/14367018/diff/17001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/14367018/diff/17001/src/hydrogen.cc#newcode906
src/hydrogen.cc:906: ASSERT(!(did_then_ ^ did_else_));
On 2013/04/23 09:27:30, mvstanton wrote:
> I'd probably comment this in english: "Make sure we handled both then and
else,
> or neither." This took me a minute to figure out.

Done.

https://codereview.chromium.org/14367018/diff/17001/src/hydrogen.cc#newcode1806
src/hydrogen.cc:1806: BuildCheckMap(value, map);
On 2013/04/23 09:27:30, mvstanton wrote:
> As I understand it, this BuildCheckMap is important because it embeds the map
as
> the first map in reloc handles for the IC. The function is used in 2 ways...in
> the inlined case, there is never a replacement done on the map. In the IC
case,
> then this map gets replaced when copying from a template.
> 
> I could imagine a BuildCompareNilScope or factory method that could ensure on
> teardown that the first handle is the "sentinel map" passed in above. But that
> is probably overkill. Or could an assert be placed right before BuildCheckMap
> that somehow makes sure there are *no* reloc handles in the code object to
this
> point? (only in IC case)
> 
> In lieu of that I'd highlight the BuildCheckMap with a comment, and also
comment
> the declaration in hydrogen.h for this special, interesting concern.

Done.

https://codereview.chromium.org/14367018/diff/17001/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/14367018/diff/17001/src/ic.cc#newcode2806
src/ic.cc:2806: if (Smi::FromInt(object->IsNull() || object->IsUndefined())) {
On 2013/04/23 09:27:30, mvstanton wrote:
> Do you intend to have Smi::FromInt(... in the if statement? Something seems
> wrong.

Done.

https://codereview.chromium.org/14367018/diff/17001/src/ic.cc#newcode2806
src/ic.cc:2806: if (Smi::FromInt(object->IsNull() || object->IsUndefined())) {
On 2013/04/23 09:27:30, mvstanton wrote:
> Do you intend to have Smi::FromInt(... in the if statement? Something seems
> wrong.

Done.

https://codereview.chromium.org/14367018/diff/17001/src/lithium.cc
File src/lithium.cc (right):

https://codereview.chromium.org/14367018/diff/17001/src/lithium.cc#newcode332
src/lithium.cc:332: if (block->predecessors()->length() > 1) can_eliminate =
false;
Removed.

On 2013/04/23 09:27:30, mvstanton wrote:
> What is it unique about this CL that requires this change? Is it use of blocks
> for push/pop of environment HValues?

[mvstanton, 2013-04-24 08:08:51]

Right on. lgtm.

[danno, 2013-04-24 11:32:36]

Committed patchset #10 manually as r14407 (presubmit successful).