Issue 1604002: Simple register allocation for ARM. Only top of expression...

Issue 1604002: Simple register allocation for ARM. Only top of expression... (Closed)

Created:
10 years, 8 months ago by Erik Corry

Modified:
9 years, 7 months ago

Reviewers:
Søren Thygesen Gjesse, Kasper Lund

CC:
v8-dev

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

Visibility:
Public.

Description

Simple register allocation for ARM. Only top of expression stack for now. Next step is probably fixing the binary op stubs so they can take swapped registers and fixing the deferred code so it doesn't insist that all registers except the two operands are flushed. Generates slightly worse code sometimes because the peephole push-pop elimination gets confused when we don't use the same register all the time (the old code used r0 always). Committed: http://code.google.com/p/v8/source/detail?r=4368

Patch Set 1 #

Patch Set 2 : '' #

Total comments: 72

Created: 10 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+1018 lines, -539 lines)			Patch
M	src/arm/codegen-arm.h	View	1	3 chunks	+8 lines, -4 lines	0 comments	Download
M	src/arm/codegen-arm.cc	View	1	100 chunks	+528 lines, -191 lines	28 comments	Download
M	src/arm/codegen-arm-inl.h	View	1	3 chunks	+3 lines, -5 lines	0 comments	Download
M	src/arm/register-allocator-arm.h	View	1	1 chunk	+2 lines, -1 line	0 comments	Download
M	src/arm/register-allocator-arm-inl.h	View	1	1 chunk	+0 lines, -3 lines	0 comments	Download
M	src/arm/virtual-frame-arm.h	View	1	13 chunks	+190 lines, -176 lines	18 comments	Download
M	src/arm/virtual-frame-arm.cc	View	1	4 chunks	+232 lines, -66 lines	22 comments	Download
M	src/codegen.h	View	1	1 chunk	+6 lines, -1 line	0 comments	Download
M	src/jump-target-light.cc	View		1 chunk	+2 lines, -15 lines	0 comments	Download
M	src/register-allocator.h	View	1	1 chunk	+6 lines, -1 line	4 comments	Download
M	src/virtual-frame.cc	View	1	1 chunk	+0 lines, -15 lines	0 comments	Download
M	src/virtual-frame-heavy.cc	View	1	1 chunk	+14 lines, -0 lines	0 comments	Download
M	src/virtual-frame-heavy-inl.h	View	1	1 chunk	+16 lines, -0 lines	0 comments	Download
M	src/virtual-frame-inl.h	View	1	1 chunk	+0 lines, -21 lines	0 comments	Download
M	src/virtual-frame-light.cc	View	1	1 chunk	+3 lines, -6 lines	0 comments	Download
M	src/virtual-frame-light-inl.h	View	1	1 chunk	+8 lines, -34 lines	0 comments	Download

Messages

Total messages: 5 (0 generated)

Expand Messages | Collapse Messages

Søren Thygesen Gjesse

A few drive by comments. http://codereview.chromium.org/1604002/diff/14001/15009 File src/arm/codegen-arm.cc (right): http://codereview.chromium.org/1604002/diff/14001/15009#newcode1411 src/arm/codegen-arm.cc:1411: if (!rhs.is(r0)) { Should ...

10 years, 8 months ago (2010-04-07 07:53:37 UTC) #2

Kasper Lund

LGTM from a high-level perspective. Maybe Søren could be convinced to do an even more ...

10 years, 8 months ago (2010-04-07 08:09:50 UTC) #3

Søren Thygesen Gjesse

LGTM http://codereview.chromium.org/1604002/diff/14001/15009 File src/arm/codegen-arm.cc (right): http://codereview.chromium.org/1604002/diff/14001/15009#newcode783 src/arm/codegen-arm.cc:783: case Token::ADD: // fall through. Either repeat this ...

10 years, 8 months ago (2010-04-07 10:46:49 UTC) #4

Erik Corry

10 years, 8 months ago (2010-04-07 12:49:17 UTC) #5

http://codereview.chromium.org/1604002/diff/14001/15009
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/1604002/diff/14001/15009#newcode783
src/arm/codegen-arm.cc:783: case Token::ADD:  // fall through.
On 2010/04/07 10:46:51, Søren Gjesse wrote:
> Either repeat this "fall through" comment all the way through or remove it
> (probable just remove it).

Removed.

http://codereview.chromium.org/1604002/diff/14001/15009#newcode804
src/arm/codegen-arm.cc:804: case Token::COMMA: {
On 2010/04/07 10:46:51, Søren Gjesse wrote:
> Am I missing something here, or shouldn't we make the transition, R1_R0_TOS ->
> R0_TOS or R0_R1_TOS -> R1_TOS when both TOS registers are occupied?

Yes we should.  I can't imagine that the , operator is performance critical ever
so I did it in a dumb way.  Smartened up ever so slightly.

http://codereview.chromium.org/1604002/diff/14001/15009#newcode807
src/arm/codegen-arm.cc:807: // simply discard left value
On 2010/04/07 08:09:50, Kasper Lund wrote:
> // Simply discard left value.

Done.

http://codereview.chromium.org/1604002/diff/14001/15009#newcode827
src/arm/codegen-arm.cc:827: Register tos = r0)
On 2010/04/07 08:09:50, Kasper Lund wrote:
> I would try to avoid these default arguments. Maybe it's okay in this first
> change, but I think you should try to get rid of them and make it more
explicit.

Default argument removed.

http://codereview.chromium.org/1604002/diff/14001/15009#newcode881
src/arm/codegen-arm.cc:881: if (!tos_register_.is(r0)) {
On 2010/04/07 08:09:50, Kasper Lund wrote:
> How about adding a special Move macro to the macro assembler that doesn't do
> anything when you're moving from rx to rx?

Done.  That was a nice cleanup in unrelated code too.

http://codereview.chromium.org/1604002/diff/14001/15009#newcode1028
src/arm/codegen-arm.cc:1028: 
On 2010/04/07 08:09:50, Kasper Lund wrote:
> Too much whitespace.

Done.

http://codereview.chromium.org/1604002/diff/14001/15009#newcode1095
src/arm/codegen-arm.cc:1095: deferred->Branch(ne);
On 2010/04/07 10:46:51, Søren Gjesse wrote:
> Wouldn't it be better to use VirtualFrame::scratchX() as temporary registers
> here?

Done.

http://codereview.chromium.org/1604002/diff/14001/15009#newcode1403
src/arm/codegen-arm.cc:1403: __ orr(r2, lhs, Operand(rhs));
On 2010/04/07 10:46:51, Søren Gjesse wrote:
> VirtualFrame::scratchX()?

Done.

http://codereview.chromium.org/1604002/diff/14001/15009#newcode1411
src/arm/codegen-arm.cc:1411: if (!rhs.is(r0)) {
On 2010/04/07 07:53:37, Søren Gjesse wrote:
> Should this be MacroAssembler::Swap?

Done.

http://codereview.chromium.org/1604002/diff/14001/15009#newcode1412
src/arm/codegen-arm.cc:1412: __ eor(rhs, rhs, Operand(lhs));
On 2010/04/07 08:09:50, Kasper Lund wrote:
> Add macro SwapRegisters or at least a comment here.

Done.

http://codereview.chromium.org/1604002/diff/14001/15009#newcode2860
src/arm/codegen-arm.cc:2860: int offset = FixedArray::kHeaderSize +
slot->index() * kPointerSize;
On 2010/04/07 10:46:51, Søren Gjesse wrote:
> VirtualFrame::scratchX()?

We are in a spilled scope here.

http://codereview.chromium.org/1604002/diff/14001/15009#newcode2862
src/arm/codegen-arm.cc:2862: __ RecordWrite(scratch, r3, r1);
On 2010/04/07 10:46:51, Søren Gjesse wrote:
> What about r1 here, will tos always be r0? If so at least assert it.

r1 is a scratch register, but we don't use tos any more at this point so it
doesn't matter if it gets splatted.

http://codereview.chromium.org/1604002/diff/14001/15009#newcode3216
src/arm/codegen-arm.cc:3216: VirtualFrameSmiOperation(node->binary_op(),
On 2010/04/07 07:53:37, Søren Gjesse wrote:
> VirtualFrameSmiOperation -> VirtualFrameSmiCompoundAssignmentOperation? rather
> long though...

I think that's too long.  The non-virtual frame based one is just called
SmiOperation.

http://codereview.chromium.org/1604002/diff/14001/15009#newcode4574
src/arm/codegen-arm.cc:4574: // Load the operand, move it to register r0 or r1.
On 2010/04/07 07:53:37, Søren Gjesse wrote:
> This comment implies that PopToRegister will always use r0 or r1. Make it more
> general?

We didn't actually use the fact that it is always r0 or r1.  Comment rewritten.

http://codereview.chromium.org/1604002/diff/14001/15011
File src/arm/virtual-frame-arm.cc (right):

http://codereview.chromium.org/1604002/diff/14001/15011#newcode43
src/arm/virtual-frame-arm.cc:43: MergeTo(&where_to_go);
On 2010/04/07 10:46:51, Søren Gjesse wrote:
> Maybe add a comment here saying the r0/r1 TOS state is now "detached" from the
> frame.

Done.

http://codereview.chromium.org/1604002/diff/14001/15011#newcode67
src/arm/virtual-frame-arm.cc:67: void VirtualFrame::MergeTo(VirtualFrame*
expected) {
On 2010/04/07 08:09:50, Kasper Lund wrote:
> Do you have good tests of this function? Full coverage?

No.  The coverage tool for covering code generated by a given line isn't working
right now.  I read it through once more and found a bug in it.  In the slightly
longer term this needs to be done differently anyway and this code will go away.

http://codereview.chromium.org/1604002/diff/14001/15011#newcode69
src/arm/virtual-frame-arm.cc:69: switch (top_of_stack_state_ * 5 +
expected->top_of_stack_state_) {
On 2010/04/07 10:46:51, Søren Gjesse wrote:
> Maybe add a macro for (one_state * 5 + another_state) and use the actual
> constant instead of 5. Maybe use shift of 3 instead.

On ARM multiplying by 5 should be the same speed as shifting by 3 (it's a
shifted add instruction) and it keeps the table dense.  Created a macro.

http://codereview.chromium.org/1604002/diff/14001/15011#newcode129
src/arm/virtual-frame-arm.cc:129: case R1_R0_TOS * 5 + R0_R1_TOS:
On 2010/04/07 07:53:37, Søren Gjesse wrote:
> MacroAssembler::Swap?

Done.

http://codereview.chromium.org/1604002/diff/14001/15011#newcode146
src/arm/virtual-frame-arm.cc:146: __ eor(r0, r0, Operand(r1));
On 2010/04/07 08:09:50, Kasper Lund wrote:
> Swap again.

Done.

http://codereview.chromium.org/1604002/diff/14001/15011#newcode151
src/arm/virtual-frame-arm.cc:151: break;
On 2010/04/07 10:46:51, Søren Gjesse wrote:
> default with UNREACHABLE()?

Done.

http://codereview.chromium.org/1604002/diff/14001/15011#newcode307
src/arm/virtual-frame-arm.cc:307: 
On 2010/04/07 10:46:51, Søren Gjesse wrote:
> Lining up the states in a comment here could be helpful.
> 
>   NO_TOS_REGISTERS
>   R0_TOS
>   R1_TOS
>   R0_R1_TOS
>   R1_R0_TOS
> 

Done.

http://codereview.chromium.org/1604002/diff/14001/15011#newcode379
src/arm/virtual-frame-arm.cc:379: Register VirtualFrame::PopToRegister(Register
but_not_to_this_one) {
On 2010/04/07 10:46:51, Søren Gjesse wrote:
> Maybe assert that but_not_to_this_one is one of the tos registers (otherwise
it
> does not make much sense, and is probably a mistake).

Done.

http://codereview.chromium.org/1604002/diff/14001/15011#newcode390
src/arm/virtual-frame-arm.cc:390: } else {
On 2010/04/07 10:46:51, Søren Gjesse wrote:
> Isn't but_not_to_this_one ignored in the else part?

Yes, because it never happens.

http://codereview.chromium.org/1604002/diff/14001/15011#newcode392
src/arm/virtual-frame-arm.cc:392: top_of_stack_state_ =
kPopState[top_of_stack_state_];
On 2010/04/07 10:46:51, Søren Gjesse wrote:
> ASSERT(!answer.is(but_not_to_this_one));

Done.

http://codereview.chromium.org/1604002/diff/14001/15011#newcode413
src/arm/virtual-frame-arm.cc:413: }
On 2010/04/07 10:46:51, Søren Gjesse wrote:
> Maybe add some kind of helper function "EnsureOneFreeTOSRegister", as kind of
> the same code used here is also used in a couple of places below.

Done.

http://codereview.chromium.org/1604002/diff/14001/15012
File src/arm/virtual-frame-arm.h (right):

http://codereview.chromium.org/1604002/diff/14001/15012#newcode54
src/arm/virtual-frame-arm.h:54: inline SpilledScope(VirtualFrame* frame)
On 2010/04/07 08:09:50, Kasper Lund wrote:
> Why put inline here? It needs to be explicit though.

Done.

http://codereview.chromium.org/1604002/diff/14001/15012#newcode65
src/arm/virtual-frame-arm.h:65: inline ~SpilledScope() {
On 2010/04/07 08:09:50, Kasper Lund wrote:
> Why put inline here?

Done.

http://codereview.chromium.org/1604002/diff/14001/15012#newcode68
src/arm/virtual-frame-arm.h:68: static inline bool is_spilled() { return
is_spilled_; }
On 2010/04/07 08:09:50, Kasper Lund wrote:
> Why put inline here?

Done.

http://codereview.chromium.org/1604002/diff/14001/15012#newcode82
src/arm/virtual-frame-arm.h:82: // is not spilled, ie where register allocation
occurs.  Eventually
On 2010/04/07 07:53:37, Søren Gjesse wrote:
> Dot after ie?

Done.

http://codereview.chromium.org/1604002/diff/14001/15012#newcode183
src/arm/virtual-frame-arm.h:183: case R0_R1_TOS:
On 2010/04/07 07:53:37, Søren Gjesse wrote:
> Maybe it is just me, but my initial assumption was that r0 was above r1 on
stack
> when seeing R0_R1_TOS, but it is probably just a matter of getting used to it.
> Making it the other way would probably confuse somebody else.

Reversed.  Lets see how that works.

http://codereview.chromium.org/1604002/diff/14001/15012#newcode203
src/arm/virtual-frame-arm.h:203: inline void AssertIsSpilled() {
On 2010/04/07 08:09:50, Kasper Lund wrote:
> Remove inline?

Done.

http://codereview.chromium.org/1604002/diff/14001/15012#newcode208
src/arm/virtual-frame-arm.h:208: inline void AssertIsNotSpilled() {
On 2010/04/07 08:09:50, Kasper Lund wrote:
> Remove inline?

Done.

http://codereview.chromium.org/1604002/diff/14001/15012#newcode383
src/arm/virtual-frame-arm.h:383: static inline Register scratch0() { return r7;
}
On 2010/04/07 08:09:50, Kasper Lund wrote:
> inline?

Done.

http://codereview.chromium.org/1604002/diff/14001/15012#newcode399
src/arm/virtual-frame-arm.h:399: 
On 2010/04/07 10:46:51, Søren Gjesse wrote:
> Most of the names of these constants are self-explanatory, but I did stumble a
> bit on kPopState and kPushState. Maybe lengthening the name might help, e.g.
> 
>   kPopState -> kStateAfterPop
>   kPushState ->
>
kStateAfterPush(LoosingTheBottomRegisterWhichIsAssumedToHaveBeenReallyPushedBeforehand)

Done (with a rename and a comment in the .cc file).

http://codereview.chromium.org/1604002/diff/14001/15001
File src/register-allocator.h (right):

http://codereview.chromium.org/1604002/diff/14001/15001#newcode218
src/register-allocator.h:218: static const int kNumRegisters =
On 2010/04/07 10:46:51, Søren Gjesse wrote:
> 4 space indent.

Done.

http://codereview.chromium.org/1604002/diff/14001/15001#newcode219
src/register-allocator.h:219: (RegisterAllocatorConstants::kNumRegisters == 0 ?
On 2010/04/07 10:46:51, Søren Gjesse wrote:
> 1 : RegisterAllocatorConstants::kNumRegisters on same line?
> 
> The expression does not need to be in ()'s - or move the last ) to just after
==
> 0.

Done.

Expand Messages | Collapse Messages