Issue 6893156: Unified CallWrapper and PostCallGenerator classes, the former is a

Issue 6893156: Unified CallWrapper and PostCallGenerator classes, the former is a (Closed)

Created:
9 years, 7 months ago by Sven Panne

Modified:
9 years, 7 months ago

Reviewers:
Kevin Millikin (Chromium), fschneider

CC:
v8-dev

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

Visibility:
Public.

Description

Unified CallWrapper and PostCallGenerator classes, the former is a generalization of the latter. This makes CallWrapper architecture-independant, so it can be pulled up into assembler.h, nuking 3 copy-n-paste classes. Only a small improvement, but nevertheless...

Patch Set 1 #

Total comments: 6

Patch Set 2 : '' #

Created: 9 years, 7 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+86 lines, -96 lines)			Patch
M	src/arm/macro-assembler-arm.h	View	1	2 chunks	+0 lines, -18 lines	0 comments	Download
M	src/assembler.h	View	1	1 chunk	+14 lines, -0 lines	0 comments	Download
M	src/ia32/assembler-ia32.h	View	1	1 chunk	+2 lines, -0 lines	0 comments	Download
M	src/ia32/assembler-ia32.cc	View	1	1 chunk	+12 lines, -0 lines	0 comments	Download
M	src/ia32/lithium-codegen-ia32.cc	View	1	2 chunks	+4 lines, -2 lines	0 comments	Download
M	src/ia32/macro-assembler-ia32.h	View	1	4 chunks	+6 lines, -20 lines	0 comments	Download
M	src/ia32/macro-assembler-ia32.cc	View	1	8 chunks	+20 lines, -16 lines	0 comments	Download
M	src/mips/macro-assembler-mips.h	View	1	6 chunks	+4 lines, -16 lines	0 comments	Download
M	src/mips/macro-assembler-mips.cc	View	1	7 chunks	+24 lines, -8 lines	0 comments	Download
M	src/x64/macro-assembler-x64.h	View	1	2 chunks	+0 lines, -16 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

Kevin Millikin (Chromium)

LGTM, nice cleanup. http://codereview.chromium.org/6893156/diff/1/src/ia32/assembler-ia32.cc File src/ia32/assembler-ia32.cc (right): http://codereview.chromium.org/6893156/diff/1/src/ia32/assembler-ia32.cc#newcode1584 src/ia32/assembler-ia32.cc:1584: return 1 /* EMIT */ + ...

9 years, 7 months ago (2011-05-03 08:43:19 UTC) #2

Sven Panne

9 years, 7 months ago (2011-05-03 09:09:54 UTC) #3

http://codereview.chromium.org/6893156/diff/1/src/ia32/assembler-ia32.cc
File src/ia32/assembler-ia32.cc (right):

http://codereview.chromium.org/6893156/diff/1/src/ia32/assembler-ia32.cc#newc...
src/ia32/assembler-ia32.cc:1584: return 1 /* EMIT */ + adr.len_ /* emit_operand
*/;
On 2011/05/03 08:43:19, Kevin Millikin wrote:
> I find the inline comments distracting here.  I'd rather see:
> 
> // Call size is 1 (opcode) + adr.len_ (operand).
> return 1 + adr.len_;

Done.

http://codereview.chromium.org/6893156/diff/1/src/ia32/assembler-ia32.cc#newc...
src/ia32/assembler-ia32.cc:1593: emit_operand(edx, adr);
On 2011/05/03 08:43:19, Kevin Millikin wrote:
> You could ASSERT(pc_ - last_pc_ == CallSize(adr)), just to make something
break if the encoding of call was changed here and CallSize was not updated.

Good idea, CallSize is a bit fragile.

http://codereview.chromium.org/6893156/diff/1/src/ia32/macro-assembler-ia32.cc
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/6893156/diff/1/src/ia32/macro-assembler-ia32.c...
src/ia32/macro-assembler-ia32.cc:1531: flag, call_wrapper);
On 2011/05/03 08:43:19, Kevin Millikin wrote:
> This line may fit on the line before.

Done.

Expand Messages | Collapse Messages