LGTM if comments are addressed (they're mostly nits).
Personally, I don't think the "uint32_t additional_index() const { return
hydrogen()->index_offset(); }" accessors in the LInstructions add much
value,
but you can keep them if you wish.
https://chromiumcodereview.appspot.com/10382055/diff/4019/src/arm/lithium-arm.cc
File src/arm/lithium-arm.cc (right):
https://chromiumcodereview.appspot.com/10382055/diff/4019/src/arm/lithium-arm.cc#newcode1959
src/arm/lithium-arm.cc:1959: LLoadKeyedFastElement* result =
nit: why this change?
https://chromiumcodereview.appspot.com/10382055/diff/4019/src/arm/lithium-arm.cc#newcode2039
src/arm/lithium-arm.cc:2039: return new(zone())
LStoreKeyedFastDoubleElement(elements,
nit: why this change?
https://chromiumcodereview.appspot.com/10382055/diff/4019/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):
https://chromiumcodereview.appspot.com/10382055/diff/4019/src/arm/lithium-codegen-arm.cc#newcode2751
src/arm/lithium-codegen-arm.cc:2751: __ ldr(result,
Formatting proposal:
uint32_t offset = FixedArray::kHeaderSize +
(instr->additional_index() << kPointerSizeLog2);
__ ldr(result, FieldMemOperand(scratch, offset));
https://chromiumcodereview.appspot.com/10382055/diff/4019/src/arm/lithium-codegen-arm.cc#newcode2794
src/arm/lithium-codegen-arm.cc:2794: + (instr->additional_index() <<
shift_size)));
nit: for consistency, the operator goes at the end of the last line.
https://chromiumcodereview.appspot.com/10382055/diff/4019/src/arm/lithium-codegen-arm.cc#newcode2844
src/arm/lithium-codegen-arm.cc:2844: constant_key * (1 << shift_size) +
additional_offset)
nit: drop the "* (1". Just "(constant_key << shift_size)". It's cleaner.
https://chromiumcodereview.appspot.com/10382055/diff/4019/src/arm/lithium-codegen-arm.cc#newcode3746
src/arm/lithium-codegen-arm.cc:3746: * kPointerSize +
FixedArray::kHeaderSize;
nit: looks like "* kPointerSize" might just fit onto the line before,
which would make more sense.
https://chromiumcodereview.appspot.com/10382055/diff/4019/src/arm/lithium-codegen-arm.cc#newcode3860
src/arm/lithium-codegen-arm.cc:3860: ? MemOperand(external_pointer, key,
LSL, shift_size)
nit: 4 space indent please.
https://chromiumcodereview.appspot.com/10382055/diff/4019/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):
https://chromiumcodereview.appspot.com/10382055/diff/4019/src/hydrogen-instructions.h#newcode3967
src/hydrogen-instructions.h:3967: : hole_check_mode_(hole_check_mode),
index_offset_(0) {
how about ", is_dehoisted_(false)"?
https://chromiumcodereview.appspot.com/10382055/diff/4019/src/hydrogen-instructions.h#newcode4001
src/hydrogen-instructions.h:4001: return hole_check_mode_ ==
other_load->hole_check_mode_;
I think you need to add a comparison of index_offset_ here. (Same for
the other classes.)
https://chromiumcodereview.appspot.com/10382055/diff/4019/src/hydrogen.cc
File src/hydrogen.cc (right):
https://chromiumcodereview.appspot.com/10382055/diff/4019/src/hydrogen.cc#newcode3084
src/hydrogen.cc:3084: Counters* counters) {
No need to pass in |counters| any more, is there?
https://chromiumcodereview.appspot.com/10382055/diff/4019/src/hydrogen.cc#newcode3086
src/hydrogen.cc:3086: array_operation->SetDehoisted(false);
It's cleaner to do this in the instruction's c'tor (see comment there).
https://chromiumcodereview.appspot.com/10382055/diff/4019/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):
https://chromiumcodereview.appspot.com/10382055/diff/4019/src/ia32/lithium-ia32.cc#newcode1961
src/ia32/lithium-ia32.cc:1961: LLoadKeyedFastElement* result =
nit: why this change?
https://chromiumcodereview.appspot.com/10382055/diff/4019/src/v8-counters.h
File src/v8-counters.h (right):
https://chromiumcodereview.appspot.com/10382055/diff/4019/src/v8-counters.h#newcode1
src/v8-counters.h:1: // Copyright 2011 the V8 project authors. All
rights reserved.
nit: 2012
https://chromiumcodereview.appspot.com/10382055/diff/4019/src/x64/lithium-codegen-x64.cc
File src/x64/lithium-codegen-x64.cc (right):
https://chromiumcodereview.appspot.com/10382055/diff/4019/src/x64/lithium-codegen-x64.cc#newcode2378
src/x64/lithium-codegen-x64.cc:2378: if
(instr->hydrogen()->IsDehoisted()) {
Doesn't this need an additional "&& !instr->key()->IsConstantOperand()"?
Same question five more times below.
https://chromiumcodereview.appspot.com/10382055/diff/4019/src/x64/lithium-codegen-x64.cc#newcode2380
src/x64/lithium-codegen-x64.cc:2380: // and the dehoisted address
computation happens in 64 bits
nit: please end the sentence with a full stop.
https://chromiumcodereview.appspot.com/10382055/diff/4019/src/x64/lithium-codegen-x64.cc#newcode3460
src/x64/lithium-codegen-x64.cc:3460: - kHeapObjectTag,
nit: I'd change the indentation to:
Operand operand =
BuildFastArrayOperand(instr->object(),
instr->key(),
FAST_ELEMENTS,
FixedArray::kHeaderSize - kHeapObjectTag,
instr->additional_index());
https://chromiumcodereview.appspot.com/10382055/diff/4019/src/x64/lithium-x64.cc
File src/x64/lithium-x64.cc (right):
https://chromiumcodereview.appspot.com/10382055/diff/4019/src/x64/lithium-x64.cc#newcode1966
src/x64/lithium-x64.cc:1966: return new(zone())
nit: why this change?
https://chromiumcodereview.appspot.com/10382055/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
