https://codereview.chromium.org/13426006/diff/7001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):
https://codereview.chromium.org/13426006/diff/7001/src/hydrogen-instructions.h#newcode3232
src/hydrogen-instructions.h:3232: return !representation().IsDouble() ||
ImmortalImmovable();
Maybe it's better to factor our the special cases for doubles into
another predicate, like IsSpecialDouble() that is used both here and in
ImmortalImmovable.
https://codereview.chromium.org/13426006/diff/7001/src/hydrogen-instructions.h#newcode5689
src/hydrogen-instructions.h:5689:
HConstant::cast(value())->IsDoubleCanonicalHole();
This predicate seems misleading... it checks only for the double
canonical hold, but not the real hole. Either rename the predicate or
add support for testing for the the_hole constant tagged value.
https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):
https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-codegen-ia32.cc#newcode374
src/ia32/lithium-codegen-ia32.cc:374: if
(instr->ClobbersDoubleRegisters()) {
I am not sure that is safe to do _after_ the CompileToNative. What if
instr is a call? That could call other JavaScript code that manipulates
the FP stack, and there would be something on the stack. I think this
should be before CompileToNative, checking instr to see if any of its
inputs are FP tos. If not, then push, then if the instruction is
ClobbersDoubleRegisters or HadDoubleRegisterResult, then it should
PopX87 if the stack isn't empty.
This strategy would also let you properly handle Gaps... if you never
"pop" from a gap, then this code makes sure that the pop happens before
the next instruction, or not at all if the next instruction uses the
value.
https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-codegen-ia32.cc#newcode561
src/ia32/lithium-codegen-ia32.cc:561: void
LCodeGen::PushX87Operand(Operand src) {
Maybe make this PushX87DoubleOperand?
https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-codegen-ia32.cc#newcode582
src/ia32/lithium-codegen-ia32.cc:582: void
LCodeGen::MarkReturnX87Result() {
Maybe CurrentInstructionReturnsX87Result?
https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-codegen-ia32.cc#newcode2790
src/ia32/lithium-codegen-ia32.cc:2790: __ SmiUntag(reg); // it is a smi
Is this an unrelated bug?
https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-codegen-ia32.cc#newcode3217
src/ia32/lithium-codegen-ia32.cc:3217: }
Unrelated changes?
https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-codegen-ia32.cc#newcode4292
src/ia32/lithium-codegen-ia32.cc:4292: if
(instr->value()->IsConstantOperand()) {
Since this has now gotten bigger, can you merge this in- and out- of
line property case? i.e. do the "__ mov(temp, FieldOperand(object,
JSObject::kPropertiesOffset))" first, then swap temp and object
registers? Here and on other platforms.
Also... is this change related? It's a good change.... but maybe not
here? :-)
https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-codegen-ia32.cc#newcode4533
src/ia32/lithium-codegen-ia32.cc:4533: if (instr->value()->IsRegister())
{
Is this change related to SSE2 at all?
https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-codegen-ia32.cc#newcode4547
src/ia32/lithium-codegen-ia32.cc:4547:
ASSERT(instr->value()->IsRegister());
Is this change related to SSE2 at all?
https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):
https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-ia32.cc#newcode2227
src/ia32/lithium-ia32.cc:2227: LOperand*
LChunkBuilder::GetStoreKeyedValueOperand(HStoreKeyed* instr) {
Is this change related?
https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-ia32.cc#newcode2381
src/ia32/lithium-ia32.cc:2381: : UseRegisterOrConstant(instr->value());
Is this change related?
https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-ia32.cc#newcode2520
src/ia32/lithium-ia32.cc:2520: Register reg;
unrelated change?
https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-ia32.cc#newcode2560
src/ia32/lithium-ia32.cc:2560: LOperand* length;
unrelated change?
https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-ia32.h
File src/ia32/lithium-ia32.h (right):
https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-ia32.h#newcode1408
src/ia32/lithium-ia32.h:1408: DECLARE_HYDROGEN_ACCESSOR(Return)
Unrelated change?
https://codereview.chromium.org/13426006/diff/7001/src/ia32/lithium-ia32.h#newcode2893
src/ia32/lithium-ia32.h:2893: LOperand*
GetStoreKeyedValueOperand(HStoreKeyed* instr);
Is this change related?
https://codereview.chromium.org/13426006/
--
--
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.
