Issue 16361015: Migrate Compare ICs to new type rep

Issue 16361015: Migrate Compare ICs to new type rep (Closed)

Created:
7 years, 6 months ago by rossberg

Modified:
7 years, 6 months ago

Reviewers:
Jakob Kummerow

CC:
v8-dev

Base URL:
https://v8.googlecode.com/svn/branches/bleeding_edge

Visibility:
Public.

More Reviews

Description

Migrate Compare ICs to new type rep (Does not yet use common AST expression type field.) R=jkummerow@chromium.org BUG= Committed: http://code.google.com/p/v8/source/detail?r=15093

Patch Set 13 : Comments #

Created: 7 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+451 lines, -317 lines)			Patch
M	src/ast.h	View	1 2 3 4 5 6 7	5 chunks	+13 lines, -23 lines	0 comments	Download
M	src/ast.cc	View	1 2 3 4 5 6 7	3 chunks	+6 lines, -24 lines	0 comments	Download
	src/code-stubs.h	View	1	4 chunks	+23 lines, -21 lines	0 comments	Download
M	src/code-stubs.cc	View	1 2	3 chunks	+39 lines, -9 lines	0 comments	Download
M	src/code-stubs-hydrogen.cc	View	1	1 chunk	+5 lines, -4 lines	0 comments	Download
M	src/compiler.cc	View	1 2 3 4 5 6 7	1 chunk	+1 line, -1 line	0 comments	Download
M	src/hydrogen.h	View	1 2 3 4 5 6 7	2 chunks	+2 lines, -2 lines	0 comments	Download
M	src/hydrogen.cc	View	1 2 3 4 5 6 7	10 chunks	+30 lines, -27 lines	0 comments	Download
M	src/ic.h	View	1	1 chunk	+3 lines, -0 lines	0 comments	Download
M	src/ic.cc	View	1 2	5 chunks	+38 lines, -10 lines	0 comments	Download
M	src/objects.h	View	1 2 3 4 5 6 7	1 chunk	+1 line, -1 line	0 comments	Download
M	src/objects.cc	View	1 2 3 4 5 6 7	1 chunk	+1 line, -1 line	0 comments	Download
M	src/type-info.h	View	1 2 3 4 5 6 7	6 chunks	+13 lines, -15 lines	0 comments	Download
M	src/type-info.cc	View	1 2 3 4 5 6 7	5 chunks	+43 lines, -106 lines	0 comments	Download
M	src/types.h	View	1 2	8 chunks	+76 lines, -20 lines	0 comments	Download
M	src/types.cc	View	1 2	9 chunks	+103 lines, -9 lines	0 comments	Download
M	src/typing.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	src/typing.cc	View		1 chunk	+1 line, -1 line	0 comments	Download
M	test/cctest/test-compare-nil-ic-stub.cc	View	1	2 chunks	+22 lines, -20 lines	0 comments	Download
M	test/cctest/test-types.cc	View		12 chunks	+30 lines, -22 lines	0 comments	Download

Messages

Total messages: 4 (0 generated)

Expand Messages | Collapse Messages

Jakob Kummerow

LGTM with nits. https://codereview.chromium.org/16361015/diff/3001/src/code-stubs.cc File src/code-stubs.cc (right): https://codereview.chromium.org/16361015/diff/3001/src/code-stubs.cc#newcode440 src/code-stubs.cc:440: object->IsOddball() || nit: I kinda preferred ...

7 years, 6 months ago (2013-06-11 17:58:07 UTC) #2

rossberg

Committed patchset #13 manually as r15093 (presubmit successful).

7 years, 6 months ago (2013-06-12 17:20:57 UTC) #3

rossberg

7 years, 6 months ago (2013-06-12 17:21:32 UTC) #4

Message was sent while issue was closed.

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

https://codereview.chromium.org/16361015/diff/3001/src/code-stubs.cc#newcode440
src/code-stubs.cc:440: object->IsOddball() ||
On 2013/06/11 17:58:07, Jakob wrote:
> nit: I kinda preferred the old indentation here, but you can do as you wish.

Done. (That actually was an unintended side-effect of using Ctrl-I in Eclipse.)

https://codereview.chromium.org/16361015/diff/3001/src/code-stubs.cc#newcode490
src/code-stubs.cc:490: Isolate* isolate, State state, Handle<Map> map) {
On 2013/06/11 17:58:07, Jakob wrote:
> nit: one line per argument please

Done, but, seriously? :)

https://codereview.chromium.org/16361015/diff/3001/src/hydrogen.cc
File src/hydrogen.cc (left):

https://codereview.chromium.org/16361015/diff/3001/src/hydrogen.cc#oldcode1719
src/hydrogen.cc:1719: ASSERT(!types.Contains(CompareNilICStub::UNDETECTABLE) ||
On 2013/06/11 17:58:07, Jakob wrote:
> I think you could preserve this ASSERT:
> 
>   if (type->Maybe(Type::Undetectable())) {
>     ASSERT(type->NumClasses() == 0);
>     ...

No, as discussed offline, this would no longer be correct once the type
information flowing into this function is derived from anything else but the
stub state alone.

https://codereview.chromium.org/16361015/diff/3001/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/16361015/diff/3001/src/ic.cc#newcode2776
src/ic.cc:2776: Isolate* isolate, CompareIC::State state, Handle<Map> map) {
On 2013/06/11 17:58:07, Jakob wrote:
> nit: for method definitions/declarations (as opposed to calls), one line per
> argument please (unless the entire signature fits on one line).

Done.

https://codereview.chromium.org/16361015/diff/3001/src/types.h
File src/types.h (right):

https://codereview.chromium.org/16361015/diff/3001/src/types.h#newcode95
src/types.h:95: static Type* Boxed() { return from_bitset(kBoxed); }
On 2013/06/11 17:58:07, Jakob wrote:
> Why not "HeapObject"? I don't care much either way, but HeapObject is the term
> we've been using so far for "anything that has a heap object tag".

Produces annoying name clashes between type and function in the class. Chose
"Allocated" instead.

https://codereview.chromium.org/16361015/diff/3001/src/types.h#newcode149
src/types.h:149: Handle<T> Get();
On 2013/06/11 17:58:07, Jakob wrote:
> nit: most of our iterators use the names "Current()" and "Advance()". (I don't
> feel very strongly about it, but consistency is nice and "Advance()" is
arguably
> a better name than "Next()" -- I associate "next()" with iterators that
actually
> return the next element from that function.)

Done.

Expand Messages | Collapse Messages