Another round of cleanup:
- Fixes on ARM
- Fixed indentation.
- Removed unnecessary {}-blocks.
- Fixed calls to runtime to restore eax.
- Made comment more consistent.
http://codereview.chromium.org/316009/diff/7002/7008
File src/arm/fast-codegen-arm.cc (right):
http://codereview.chromium.org/316009/diff/7002/7008#newcode228
Line 228: // If so, jump to the deferred code.
On 2009/10/24 07:30:08, Kevin Millikin wrote:
> The comment about deferred code is out of date.
Done.
http://codereview.chromium.org/316009/diff/7002/7008#newcode242
Line 242: __ push(r0);
On 2009/10/24 07:30:08, Kevin Millikin wrote:
> There's an opportunity to combine the three pushes into a stm by
choosing a
> higher register index than r1 for the literals array. I think you can
just swap
> the use of r1 and r2 in the code before and including the undefined
check.
Done.
http://codereview.chromium.org/316009/diff/7002/7008#newcode255
Line 255: __ push(r0);
On 2009/10/24 07:30:08, Kevin Millikin wrote:
> I think you can skip this push and use code more like the ia32,
keeping the
> object in r0 for each spin of the code generation loop.
Added a result_saved variable so that we only save the result on the
stack if it is needed.
http://codereview.chromium.org/316009/diff/7002/7008#newcode268
Line 268: case ObjectLiteral::Property::PROTOTYPE: {
On 2009/10/24 07:30:08, Kevin Millikin wrote:
> No need for block.
Done.
http://codereview.chromium.org/316009/diff/7002/7008#newcode275
Line 275: Visit(value);
On 2009/10/24 07:30:08, Kevin Millikin wrote:
> Assert this is on stack (location is_temporary for now).
Done.
http://codereview.chromium.org/316009/diff/7002/7008#newcode277
Line 277: __ ldr(r0, MemOperand(sp));
On 2009/10/24 07:30:08, Kevin Millikin wrote:
> The value restored to r0 from the stack is the same as the one already
in r0
> returned by the runtime.
Done. SetProperty return the value not the receiver in r0. So we have to
save r0 in this case. I fixed this also on ia32 and x64.
http://codereview.chromium.org/316009/diff/7002/7008#newcode281
Line 281: case ObjectLiteral::Property::GETTER: {
On 2009/10/24 07:30:08, Kevin Millikin wrote:
> No need for block.
Done.
http://codereview.chromium.org/316009/diff/7002/7008#newcode292
Line 292: Visit(value);
On 2009/10/24 07:30:08, Kevin Millikin wrote:
> Assert this is on stack.
Done.
http://codereview.chromium.org/316009/diff/7002/7008#newcode294
Line 294: __ ldr(r0, MemOperand(sp));
On 2009/10/24 07:30:08, Kevin Millikin wrote:
> Same comment. I don't think there is a need to save the object on the
stack or
> restore it.
Same comment as for SetProperty applies here.
http://codereview.chromium.org/316009/diff/7002/7008#newcode299
Line 299: if (expr->location().is_nowhere()) {
On 2009/10/24 07:30:08, Kevin Millikin wrote:
> Here you would do nothing for nowhere and push r0 for temporary.
Done. I added a result_saved variable that determines if we have to
pop() here or do nothing.
http://codereview.chromium.org/316009/diff/7002/7004
File src/ia32/fast-codegen-ia32.cc (right):
http://codereview.chromium.org/316009/diff/7002/7004#newcode245
Line 245: bool result_saved = false;
On 2009/10/24 07:30:08, Kevin Millikin wrote:
> I think the code below reads a little better if you call this
'object_in_eax' or
> 'object_in_reg' or something instead. (Ie, reverse the sense of the
boolean.)
I left result_saved unchanged for now to be consistent with the other
Visit-functions where we use the same name.
http://codereview.chromium.org/316009/diff/7002/7004#newcode256
Line 256: if (!result_saved) {
On 2009/10/24 07:30:08, Kevin Millikin wrote:
> if (object_in_eax) {
> __ push(eax);
> object_in_eax = false;
> }
Unchanged. See my other comment about the result_saved variable.
http://codereview.chromium.org/316009/diff/7002/7004#newcode261
Line 261: case ObjectLiteral::Property::MATERIALIZED_LITERAL: // fall
through
On 2009/10/24 07:30:08, Kevin Millikin wrote:
> We indent case labels two spaces (and their bodies a further two
spaces).
Done. (also for x64)
http://codereview.chromium.org/316009/diff/7002/7004#newcode263
Line 263: case ObjectLiteral::Property::COMPUTED: {
On 2009/10/24 07:30:08, Kevin Millikin wrote:
> No need for a block around this case.
Done. (also for x64)
http://codereview.chromium.org/316009/diff/7002/7004#newcode276
Line 276: case ObjectLiteral::Property::PROTOTYPE: {
On 2009/10/24 07:30:08, Kevin Millikin wrote:
> No need for a block around this one.
Done.
http://codereview.chromium.org/316009/diff/7002/7004#newcode285
Line 285: result_saved = false;
On 2009/10/24 07:30:08, Kevin Millikin wrote:
> object_in_eax = true;
Unchanged. See my other comment about the result_saved variable.
http://codereview.chromium.org/316009/diff/7002/7004#newcode289
Line 289: case ObjectLiteral::Property::GETTER: {
On 2009/10/24 07:30:08, Kevin Millikin wrote:
> No need for block.
Done. (also for x64)
http://codereview.chromium.org/316009
--~--~---------~--~----~------------~-------~--~----~
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
-~----------~----~----~----~------~----~------~--~---
