Thanks for the comments Danno, here are my changes.
https://codereview.chromium.org/11238016/diff/4024/src/arm/lithium-codegen-arm.cc File src/arm/lithium-codegen-arm.cc (right): https://codereview.chromium.org/11238016/diff/4024/src/arm/lithium-codegen-arm.cc#newcode3001 src/arm/lithium-codegen-arm.cc:3001: void LCodeGen::DoLoadKeyedDouble(LLoadKeyed* instr) { On 2012/10/24 11:42:43, danno wrote:
nit: Just to make searches easier, maybe call this
DoLoadKeyedFixedDouble. Even
better, call this DoLoadKeyedFixedDoubleArray and the other one DoLoadKeyedFixedArray
Done. https://codereview.chromium.org/11238016/diff/4024/src/hydrogen-instructions.h File src/hydrogen-instructions.h (right): https://codereview.chromium.org/11238016/diff/4024/src/hydrogen-instructions.h#newcode4285 src/hydrogen-instructions.h:4285: SetGVNFlag(kDependsOnCalls); On 2012/10/24 11:42:43, danno wrote:
Technically, I think the call to SetGVNFlag(kDependsOnCalls) isn't
necessary,
but that's a deeper discussion outside the scope of this CL. Ask it
about me
when you get back, it might be a second cleanup CL.
okay, will do. https://codereview.chromium.org/11238016/diff/4024/src/hydrogen-instructions.h#newcode4349 src/hydrogen-instructions.h:4349: // Is this okay? On 2012/10/24 11:42:43, danno wrote:
That's fine. But crate a constant for the number of bits you use and
add a
static assert here that makes sure that it is enough to hold
ElementsKind, e.g.
STATIC_ASSERT(kElementsKindCount <= (1 << kBitsForElementsKind))
Done. https://codereview.chromium.org/11238016/diff/4024/src/ia32/lithium-codegen-ia32.cc File src/ia32/lithium-codegen-ia32.cc (right): https://codereview.chromium.org/11238016/diff/4024/src/ia32/lithium-codegen-ia32.cc#newcode2866 src/ia32/lithium-codegen-ia32.cc:2866: On 2012/10/24 11:42:43, danno wrote:
nit: two spaces
Done. https://codereview.chromium.org/11238016/diff/4024/src/ia32/lithium-codegen-ia32.cc#newcode3911 src/ia32/lithium-codegen-ia32.cc:3911: Register key = instr->key()->IsRegister() ? ToRegister(instr->key()) On 2012/10/24 11:42:43, danno wrote:
I think this still fits on one line?
Done. https://codereview.chromium.org/11238016/diff/4024/src/ia32/lithium-ia32.h File src/ia32/lithium-ia32.h (right): https://codereview.chromium.org/11238016/diff/4024/src/ia32/lithium-ia32.h#newcode1395 src/ia32/lithium-ia32.h:1395: LOperand* object() { return inputs_[0]; } On 2012/10/24 11:42:43, danno wrote:
I think elements is actually better. No matter which variant, external
or not,
this value is the "base" of the elements that you need to index into.
Object has
a bit of a connotation that it's any HeapObject, or even a JavaScript
object,
which it's not.
Done. And I pulled this all the way up into the hydrogen instructions too. There, I remember we had elements() in the double case, but object() in the fast case and external_pointer() in the external case. elements() does seem like the best option. https://codereview.chromium.org/11238016/diff/4024/src/ia32/lithium-ia32.h#newcode1981 src/ia32/lithium-ia32.h:1981: LOperand* object() { return inputs_[0]; } On 2012/10/24 11:42:43, danno wrote:
Change this one to elements() for symmetry with LStoreKeyed.
Done. https://codereview.chromium.org/11238016/diff/4024/src/x64/lithium-x64.h File src/x64/lithium-x64.h (right): https://codereview.chromium.org/11238016/diff/4024/src/x64/lithium-x64.h#newcode1365 src/x64/lithium-x64.h:1365: LOperand* object() { return inputs_[0]; } On 2012/10/24 11:42:43, danno wrote:
elements?
Done. https://codereview.chromium.org/11238016/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
