Thanks! All done. Submitted.
-- Vitaly http://codereview.chromium.org/660184/diff/1/2 File src/arm/codegen-arm.cc (right): http://codereview.chromium.org/660184/diff/1/2#newcode3439 src/arm/codegen-arm.cc:3439: __ add(r1, r1, Operand(r0)); On 2010/02/26 17:59:06, Mads Ager wrote:
I would prefer to use
__ add(r1, r1, Operand(r0, LSL, kPointerSizeLog2 - kSmiTagSize))
instead of these two instructions. Also makes it more clear to me that
you are
adding ((untagged r0) * kPointerSize) to r1.
Done. http://codereview.chromium.org/660184/diff/1/2#newcode3442 src/arm/codegen-arm.cc:3442: __ cmp(r1, Operand(Factory::undefined_value())); On 2010/02/26 17:59:06, Mads Ager wrote:
You should use LoadRoot to ip followed by cmp here to use the
RootArray instead
of embedding the object in the code.
Done. http://codereview.chromium.org/660184/diff/1/6 File src/ia32/codegen-ia32.cc (right): http://codereview.chromium.org/660184/diff/1/6#newcode5605 src/ia32/codegen-ia32.cc:5605: frame_->Dup(); On 2010/02/26 17:59:06, Mads Ager wrote:
Do you need the Dup here? You never modify code as far as I can tell,
so I
guess you could just Pop it from the frame and push it back if you
need it on
the frame. Then you can just push the result instead of overwriting
element
zero on the frame with the result.
Well, I tried this approach and couldn't make it work. After you comment I tried a bit more and found out it requires passing live values along branches. Now it works (and on x64 as well). http://codereview.chromium.org/660184/diff/1/6#newcode5625 src/ia32/codegen-ia32.cc:5625: __ Set(temp.reg(), Immediate(Factory::single_character_string_cache())); On 2010/02/26 17:59:06, Mads Ager wrote:
Adding a comment here stating that code contains a smi tagged ascii
char code
might be good to explain the times_hafl_pointer_size in the Operand
below. Done. http://codereview.chromium.org/660184/diff/1/6#newcode5629 src/ia32/codegen-ia32.cc:5629: __ mov(temp.reg(), Operand(temp.reg(), On 2010/02/26 17:59:06, Mads Ager wrote:
Use FieldOperand and drop the "- kHeapObjectTag" on the offset?
Done. http://codereview.chromium.org/660184/diff/1/12 File src/x64/codegen-x64.cc (right): http://codereview.chromium.org/660184/diff/1/12#newcode3906 src/x64/codegen-x64.cc:3906: __ movq(temp.reg(), Operand(temp.reg(), On 2010/02/26 17:59:06, Mads Ager wrote:
FieldOperand to get rid of the "- kHeapObjectTag" in the offset.
Done. http://codereview.chromium.org/660184/diff/1/12#newcode3909 src/x64/codegen-x64.cc:3909: __ Cmp(temp.reg(), Factory::undefined_value()); On 2010/02/26 17:59:06, Mads Ager wrote:
You should use CompareRoot here to use the root array.
Done. http://codereview.chromium.org/660184 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
