Issue 3666001: Defer the prefix/postfix code generation.

http://codereview.chromium.org/3666001/diff/9001/10001 File src/arm/codegen-arm.cc (right): http://codereview.chromium.org/3666001/diff/9001/10001#newcode6068 src/arm/codegen-arm.cc:6068: __ sub(value_, value_, Operand(Smi::FromInt(1)), LeaveCC, vs); The tst instruction ...

10 years, 2 months ago (2010-10-19 11:26:38 UTC) #2

http://codereview.chromium.org/3666001/diff/9001/10001
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/3666001/diff/9001/10001#newcode6068
src/arm/codegen-arm.cc:6068: __ sub(value_, value_, Operand(Smi::FromInt(1)),
LeaveCC, vs);
The tst instruction does not affect the overflow flag, so for non-Smis, which
also branch here, it will be indeterminate whether or not the optimistic
increment/decrement (which didn't happen) is reverted.

http://codereview.chromium.org/3666001/diff/9001/10001#newcode6077
src/arm/codegen-arm.cc:6077: VirtualFrame::SpilledScope spilled(frame_);
This is not how it's normally done.  See
DeferredReferenceGetNamedValue::Generate to see how to handle the virtual frame
in a deferred ::Generate() method.

You should  save the frame with something like:

  VirtualFrame copied_frame(*frame_state()->frame());
  copied_frame.SpillAll();

This will get the frame state (the description of which registers contain the
top of stack) from the point where the DeferredCode subclass was constructed. 
You are expected not to change the virtual frame between then and the point(s)
where you branch to deferred code and the point where you bind the exit of the
deferred code.  You can then restore it at the end of the deferred code with

  copied_frame.MergeTo(frame_state()->frame());

Within the ::Generate method you normally don't use EmitPush, just use __ push()
etc.

http://codereview.chromium.org/3666001/diff/9001/10001#newcode6197
src/arm/codegen-arm.cc:6197: exit.Branch(vc);
Ideally we would like to have the non-deferred case just fall through.  That
means all the code that this branch skips should be moved into the deferred code
block.

http://codereview.chromium.org/3666001/diff/9001/10001#newcode6205
src/arm/codegen-arm.cc:6205: frame_->UpdateFrameForDeferredCountOperation();
This forces the frame state to be flushed to stack (no registers contain stack
values), but it doesn't actually update the stack, just the compiler state.  It
seems wrong to me and I don't understand how it can work, except by accident. 
We are in a register allocation scope and the top of stack could be in
registers.

zhangk

Erik, Thanks for the comments. I modify the code according to your suggestions. Please let ...

10 years, 1 month ago (2010-11-04 21:07:23 UTC) #3

Erik Corry

Thanks for the update, the patch is getting better. In the notes below I have ...

10 years, 1 month ago (2010-11-05 22:04:36 UTC) #4

Thanks for the update, the patch is getting better.

In the notes below I have suggested using EmitPush instead of __ push and
RaiseHeight.  There are no deferred code objects that do that at the moment, so
there may be some reason why it doesn't work, but it seems to me that it should.
 Please let me know if I am getting confused here.  The same applies to the
suggestion to omit the SpillAll() call.

http://codereview.chromium.org/3666001/diff/20002/26001
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/3666001/diff/20002/26001#newcode6064
src/arm/codegen-arm.cc:6064: copied_frame.SpillAll();
Actually I think you don't need this, though the builtin and runtime calls will
spill all registers regardless.

http://codereview.chromium.org/3666001/diff/20002/26001#newcode6084
src/arm/codegen-arm.cc:6084: copied_frame.RaiseHeight(1, 0);
Instead of doing a "__ push" followed by RaiseHeight I think it would be cleaner
to just use __ EmitPush(), which combines both.  The InvokeBuiltin or
CallRuntime call will ensure that everything gets flushed to the stack.  This
also applies 2 places below.

http://codereview.chromium.org/3666001/diff/20002/26001#newcode6096
src/arm/codegen-arm.cc:6096: __ mov(r0, Operand(Smi::FromInt(1)));
If you use EmitPush you will have to get a new register here by using
GetTOSRegister.

http://codereview.chromium.org/3666001/diff/20002/26001#newcode6106
src/arm/codegen-arm.cc:6106: __ mov(value_, r0);
We have __ Move(value_, r0) for this purpose.

http://codereview.chromium.org/3666001/diff/20002/26001#newcode6197
src/arm/codegen-arm.cc:6197: deferred->Branch(ne);
It looks like you still have a bug here.  The tst instruction will not affect
the overflow flag, so whether or not the increment or decrement at the start of
the Generate() method gets performed will be essentially random, depending on
what the state of the overflow flag was before the tst instruction.

http://codereview.chromium.org/3666001/diff/20002/26001#newcode6209
src/arm/codegen-arm.cc:6209: // Slow case: Convert to number.  At this point the
This comment should be moved to the deferred code, where it applies.

http://codereview.chromium.org/3666001/diff/20002/26002
File src/arm/virtual-frame-arm.h (right):

http://codereview.chromium.org/3666001/diff/20002/26002#newcode402
src/arm/virtual-frame-arm.h:402: }
This change is not necessary if EmitPush is used.

zhangk

http://codereview.chromium.org/3666001/diff/20002/src/arm/codegen-arm.cc File src/arm/codegen-arm.cc (right): http://codereview.chromium.org/3666001/diff/20002/src/arm/codegen-arm.cc#newcode6064 src/arm/codegen-arm.cc:6064: copied_frame.SpillAll(); On 2010/11/05 22:04:36, Erik Corry wrote: Done. http://codereview.chromium.org/3666001/diff/20002/src/arm/codegen-arm.cc#newcode6064 ...

10 years, 1 month ago (2010-11-09 23:07:53 UTC) #5

Erik Corry

10 years, 1 month ago (2010-11-24 09:56:33 UTC) #6

This change doesn't actually run the unit tests in debug mode.  There is an
attempt to merge two incompatible frames because the frame in the deferred code
has forgotten some type information that the inline frame has retained.  I added
a call to ForgetTypeInfo which fixes this issue.  Please see also the comment in
the fixed code.

Committed as r5884.  Thankyou.

http://codereview.chromium.org/3666001/diff/32001/src/arm/codegen-arm.cc
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/3666001/diff/32001/src/arm/codegen-arm.cc#newc...
src/arm/codegen-arm.cc:6100: if (!value_.is(r0))
The 'if' is not needed.  I corrected this.

Expand Messages | Collapse Messages

Issue 3666001: Defer the prefix/postfix code generation. (Closed)

Description

Patch Set 1 : '' #

Patch Set 2 : '' #

Patch Set 3 : '' #

Messages