Very close, I like this CL a lot.
About testing: Can you make sure that all of the code paths are hit that
you've
added? i.e. add single OS::DebugBreaks one after another in
macro-assembler-ia32.cc to the different branches you've created, and run
the
unit tests to make sure that each one gets hit during the test suite run. If
not, add tests to make sure that all the code is covered.
https://codereview.chromium.org/22290005/diff/65011/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):
https://codereview.chromium.org/22290005/diff/65011/src/ia32/code-stubs-ia32.cc#newcode2668
src/ia32/code-stubs-ia32.cc:2668: true, &call_runtime);
nit: odd indentation
https://codereview.chromium.org/22290005/diff/65011/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):
https://codereview.chromium.org/22290005/diff/65011/src/ia32/lithium-codegen-ia32.cc#newcode5302
src/ia32/lithium-codegen-ia32.cc:5302: (instr->temp() != NULL) ?
ToDoubleRegister(instr->temp()) : no_xmm_reg,
nit: factor this out into a variable that is passed in into TaggedToI.
It makes debugging and readability better.
https://codereview.chromium.org/22290005/diff/65011/src/ia32/macro-assembler-ia32.cc
File src/ia32/macro-assembler-ia32.cc (right):
https://codereview.chromium.org/22290005/diff/65011/src/ia32/macro-assembler-ia32.cc#newcode407
src/ia32/macro-assembler-ia32.cc:407: j(not_equal,
!treat_minus_zero_as_zero
nit: Factor this label decision out into a variable that you pass to
this statement and the next.
https://codereview.chromium.org/22290005/diff/65011/src/ia32/macro-assembler-ia32.h
File src/ia32/macro-assembler-ia32.h (right):
https://codereview.chromium.org/22290005/diff/65011/src/ia32/macro-assembler-ia32.h#newcode478
src/ia32/macro-assembler-ia32.h:478: int index =
HeapNumber::kValueOffset - kHeapObjectTag);
nit: this parameter should be called offset, right?
https://codereview.chromium.org/22290005/diff/65011/src/ia32/macro-assembler-ia32.h#newcode485
src/ia32/macro-assembler-ia32.h:485: XMMRegister scratch, bool
treat_minus_zero_as_zero,
Can you convert this bool into an enum?
enum MinusZeroMode {
TREAT_MINUS_ZERO_AS_ZERO,
FAIL_ON_MINUS_ZERO
}
https://codereview.chromium.org/22290005/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
