Thanks guys, addressed comments. Much appreciated,
--Michael
https://codereview.chromium.org/1100083002/diff/100001/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):
https://codereview.chromium.org/1100083002/diff/100001/src/arm/lithium-codegen-arm.cc#newcode3359
src/arm/lithium-codegen-arm.cc:3359: __ LoadRoot(scratch,
Heap::kArrayProtectorRootIndex);
On 2015/04/23 16:24:37, balazs.kilvady wrote:
The result register should be used instead of scratch in LoadRoot like
in
src/arm64/lithium-codegen-arm64.cc.
Done.
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() ||
On 2015/04/23 13:40:00, Jakob wrote:
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;
Done, and I made the double case above follow the same protocol...looks
nicer.
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));
On 2015/04/23 13:40:00, Jakob wrote:
nit: s/ldr/Ldr/ (always use MacroAssembler instructions on arm64).
Done.
https://codereview.chromium.org/1100083002/diff/100001/src/arm64/lithium-codegen-arm64.cc#newcode3631
src/arm64/lithium-codegen-arm64.cc:3631: __ bind(&done);
On 2015/04/23 13:40:00, Jakob wrote:
nit: s/bind/Bind/
Done.
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 &&
On 2015/04/23 13:40:00, Jakob wrote:
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?
Indeed, good catch, fixed.
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,
On 2015/04/23 13:40:00, Jakob wrote:
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?
Okay, this is done. I was just being cautious (at the expense of memory
:p).
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 =
On 2015/04/23 13:40:00, Jakob wrote:
Suggestion: "bool holey_double_elements = *map == ..."
That would make subsequent code look a bit cleaner; but I don't feel
strongly
about it.
Done.
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());
On 2015/04/23 13:40:00, Jakob wrote:
You can collapse this into one instruction:
__ cmp(Operand::ForCell(isolate()->factory()->array_protector()),
Immediate(Smi::FromInt(1)));
Actually I can't do that for a PropertyCell, just a Cell. Cool idea
though, I didn't know about that instruction!
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)));
On 2015/04/23 13:40:00, Jakob wrote:
Please factor out the magic 0/1 constants as "static const int
kArrayProtectorValid = 1" (in class Isolate) or similar.
Done.
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()) ?
On 2015/04/23 13:40:00, Jakob wrote:
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! :-)
:D, yeah I was like wha?
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;
On 2015/04/23 13:40:00, Jakob wrote:
We used to have to initialize such variables to keep all compilers
happy... not
sure if that's still necessary.
I'll try to go without it, hopefully it'll work.
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 =
On 2015/04/23 13:40:00, Jakob wrote:
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.
That's right, and good idea to comment that. Done.
https://codereview.chromium.org/1100083002/diff/100001/src/mips/lithium-codegen-mips.cc
File src/mips/lithium-codegen-mips.cc (right):
https://codereview.chromium.org/1100083002/diff/100001/src/mips/lithium-codegen-mips.cc#newcode3295
src/mips/lithium-codegen-mips.cc:3295: __ LoadRoot(scratch,
Heap::kArrayProtectorRootIndex);
On 2015/04/23 16:24:37, balazs.kilvady wrote:
The result register should be used instead of scratch in LoadRoot like
in
src/arm64/lithium-codegen-arm64.cc.
Done.
https://codereview.chromium.org/1100083002/diff/100001/src/mips64/lithium-codegen-mips64.cc
File src/mips64/lithium-codegen-mips64.cc (right):
https://codereview.chromium.org/1100083002/diff/100001/src/mips64/lithium-codegen-mips64.cc#newcode3344
src/mips64/lithium-codegen-mips64.cc:3344: __ LoadRoot(scratch,
Heap::kArrayProtectorRootIndex);
On 2015/04/23 16:24:37, balazs.kilvady wrote:
The result register should be used instead of scratch in LoadRoot like
in
src/arm64/lithium-codegen-arm64.cc.
Done.
https://codereview.chromium.org/1100083002/diff/100001/src/mips64/lithium-codegen-mips64.cc#newcode3345
src/mips64/lithium-codegen-mips64.cc:3345: __ lw(scratch,
FieldMemOperand(result, Cell::kValueOffset));
On 2015/04/23 15:34:23, paul.l... wrote:
nit: This should probably be 'ld'. I say 'probably' since in general
values are
64-bit. But you are using just a bit flag here, so it's fine (as long
as we
never go to big-endian mips64, which we have no plans for :)
It might be worth a comment here that you are using just LS bit(s) of
value.
Done.
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.
