LGTM with a bunch of comments, mostly nits though. Happy to take another
look if
you want.
MIPS folks, can you please take a look at the lithium-codegen-mips* changes?
https://codereview.chromium.org/1100083002/diff/100001/src/arm64/lithium-arm64.cc
File src/arm64/lithium-arm64.cc (right):
https://codereview.chromium.org/1100083002/diff/100001/src/arm64/lithium-arm64.cc#newcode1727
src/arm64/lithium-arm64.cc:1727: return (instr->RequiresHoleCheck() ||
Suggestion: I'd model this after the case below (lines 1740 ff.):
LInstruction* result =
DefineAsRegister(new(zone()) LLoadKeyedFixed(elements, key,
temp));
if (instr->RequiresHoleCheck() ||
(instr->hole_mode == ... && info()->IsStub()) {
result = AssignEnvironment(result);
}
return result;
https://codereview.chromium.org/1100083002/diff/100001/src/arm64/lithium-codegen-arm64.cc
File src/arm64/lithium-codegen-arm64.cc (right):
https://codereview.chromium.org/1100083002/diff/100001/src/arm64/lithium-codegen-arm64.cc#newcode3626
src/arm64/lithium-codegen-arm64.cc:3626: __ ldr(result,
FieldMemOperand(result, Cell::kValueOffset));
nit: s/ldr/Ldr/ (always use MacroAssembler instructions on arm64).
https://codereview.chromium.org/1100083002/diff/100001/src/arm64/lithium-codegen-arm64.cc#newcode3631
src/arm64/lithium-codegen-arm64.cc:3631: __ bind(&done);
nit: s/bind/Bind/
https://codereview.chromium.org/1100083002/diff/100001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):
https://codereview.chromium.org/1100083002/diff/100001/src/code-stubs-hydrogen.cc#newcode582
src/code-stubs-hydrogen.cc:582: if (casted_stub()->elements_kind() ==
FAST_HOLEY_ELEMENTS &&
Why do we need to check for elements kind again? The IC/handler
compilers already do that when computing the convert_hole_to_undefined
flag, no?
https://codereview.chromium.org/1100083002/diff/100001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):
https://codereview.chromium.org/1100083002/diff/100001/src/hydrogen-instructions.h#newcode6614
src/hydrogen-instructions.h:6614: kBitsForBaseOffset = 25,
I think instead of moving |dehoisted| out of this bitfield, I'd rather
shrink the base offset field to 24 bits. Is there a reason not to do
that?
https://codereview.chromium.org/1100083002/diff/100001/src/hydrogen.cc
File src/hydrogen.cc (right):
https://codereview.chromium.org/1100083002/diff/100001/src/hydrogen.cc#newcode6995
src/hydrogen.cc:6995: Map* holey_double_elements =
Suggestion: "bool holey_double_elements = *map == ..."
That would make subsequent code look a bit cleaner; but I don't feel
strongly about it.
https://codereview.chromium.org/1100083002/diff/100001/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):
https://codereview.chromium.org/1100083002/diff/100001/src/ia32/lithium-codegen-ia32.cc#newcode3195
src/ia32/lithium-codegen-ia32.cc:3195: __ mov(result,
isolate()->factory()->array_protector());
You can collapse this into one instruction:
__ cmp(Operand::ForCell(isolate()->factory()->array_protector()),
Immediate(Smi::FromInt(1)));
https://codereview.chromium.org/1100083002/diff/100001/src/ia32/lithium-codegen-ia32.cc#newcode3197
src/ia32/lithium-codegen-ia32.cc:3197: Immediate(Smi::FromInt(1)));
Please factor out the magic 0/1 constants as "static const int
kArrayProtectorValid = 1" (in class Isolate) or similar.
https://codereview.chromium.org/1100083002/diff/100001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (left):
https://codereview.chromium.org/1100083002/diff/100001/src/ia32/lithium-ia32.cc#oldcode2232
src/ia32/lithium-ia32.cc:2232: if ((instr->is_external() ||
instr->is_fixed_typed_array()) ?
Yo dawg, I heard you like conditions, so I put a conditional into your
condition, so you can check a condition while you check a condition...
+1 to cleaning that up! :-)
https://codereview.chromium.org/1100083002/diff/100001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):
https://codereview.chromium.org/1100083002/diff/100001/src/ia32/lithium-ia32.cc#newcode2232
src/ia32/lithium-ia32.cc:2232: bool needs_environment;
We used to have to initialize such variables to keep all compilers
happy... not sure if that's still necessary.
https://codereview.chromium.org/1100083002/diff/100001/src/ic/ic-compiler.cc
File src/ic/ic-compiler.cc (right):
https://codereview.chromium.org/1100083002/diff/100001/src/ic/ic-compiler.cc#newcode115
src/ic/ic-compiler.cc:115: bool convert_hole_to_undefined =
Checking for a pristine array prototype is pointless here, because this
is compiled so early that it's pristine anyway, and the stub is doing
the check, right? Maybe add a comment about the latter, roughly:
// No need to check for an elements-free prototype chain here, the
generated
// stub code needs to check that dynamically anyway.
https://codereview.chromium.org/1100083002/
--
--
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/d/optout.
