Hi all,
I responded to the great comments. Two questions remain:
1) I "stole" a bit from HLoadKeyed.indexoffsetvalue so that element_kind
could
have 5 bits for it's representation. Any advice on consequences, tests,
code to
visit for this? (my question about this is still present with a
TODO(mvstanton)
in two code locations).
2) Danno, you mentioned that you had a wrong comment, related to
RequiresHoleCheck but I didn't quite understand.
Thanks,
--Michael
https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-arm.cc
File src/arm/lithium-arm.cc (right):
https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-arm.cc#newcode1875
src/arm/lithium-arm.cc:1875: if (instr->RequiresHoleCheck())
AssignEnvironment(result);
On 2012/10/21 20:44:41, danno wrote:
make sure that AssignEnvironment is the last thing done for
consistency. It
should operate on the result of DefineAsRegister(result).
Done.
https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-arm.cc#newcode1888
src/arm/lithium-arm.cc:1888: LOperand* key =
UseRegisterOrConstant(instr->key());
On 2012/10/21 20:44:41, danno wrote:
This can be UseRegisterOrConstantAtStart, I think, too, so you can
combine the
key calculate. UseRegisterOrConstantAtStart just means that it can
re-use the
register passed in for the key as the output register, if I recall
correctly,
UseRegisterOrConstant doesn't. In the arm case (and other platforms,
too), the
load result and the key aren't live at the same time for any of the
load
variants, so it's OK to use UseRegisterOrConstantAtStart.
Done.
https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-arm.cc#newcode1894
src/arm/lithium-arm.cc:1894: return (elements_kind ==
EXTERNAL_UNSIGNED_INT_ELEMENTS) ?
On 2012/10/21 20:44:41, danno wrote:
You can combine this exit sequence with the one for non external
elements:
bool can_deoptimize = instr->RequiresHoleCheck() || elements_kin ==
EXTERNAL_UNSIGNED_INT_ELEMENTS;
return can_deoptimize ? AssignEnvironment(result) : result;
Thanks, done. Indeed I could combine a lot more code in there, such as
the key creation.
https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-arm.cc#newcode1953
src/arm/lithium-arm.cc:1953: : UseRegister(instr->value());
On 2012/10/21 20:44:41, danno wrote:
Use the previous four lines to compute val in both the external and
non-external
case. in the non external case, elements_kind won't be
EXTERNAL_PIXEL_ELEMENTS
or EXTERNAL_FLOAT_ELEMENTS, so you'll get a temporary register as
expected.
Done, but there was a pretty big variation in the ia32 tree that meant I
couldn't refactor as deeply there. Have a look at that change coming up
and see what you think.
https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-arm.cc#newcode1954
src/arm/lithium-arm.cc:1954: LOperand* key =
UseRegisterOrConstant(instr->key());
On 2012/10/21 20:44:41, danno wrote:
As in load case, use LOperand* key =
UseRegisterOrConstantAtStart(instr->key());
for both branches.
Done.
https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):
https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-codegen-arm.cc#newcode2999
src/arm/lithium-codegen-arm.cc:2999:
On 2012/10/21 20:44:41, danno wrote:
nit: two line returns between functions
Done.
https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-codegen-arm.cc#newcode3006
src/arm/lithium-codegen-arm.cc:3006: bool key_is_constant =
instr->key()->IsConstantOperand();
On 2012/10/21 20:44:41, danno wrote:
Just for clarity and readability, perhaps it makes sense to factor
this part of
the if info a helper function "DoLoadKeyedFixedDoubleArray", just like
you did
for DoLoadKeyedExternal?
Done.
https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-codegen-arm.cc#newcode4068
src/arm/lithium-codegen-arm.cc:4068:
On 2012/10/21 20:44:41, danno wrote:
nit: two spaces between functions
Done.
https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-codegen-arm.cc#newcode4074
src/arm/lithium-codegen-arm.cc:4074: DwVfpRegister value =
ToDoubleRegister(instr->value());
On 2012/10/21 20:44:41, danno wrote:
Same here as above, how about DoStoreKeyedFixedDoubleArray and
DoStoreKeyedFixedArray?
Done.
https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):
https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions.cc#newcode1855
src/hydrogen-instructions.cc:1855: stream->Add("[");
On 2012/10/21 20:44:41, danno wrote:
From the above line
Done.
https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions.cc#newcode1861
src/hydrogen-instructions.cc:1861: }
On 2012/10/21 20:44:41, danno wrote:
To here can be shared at the end of the routine. RequiresHoleCheck
should always
return false for external arrays, so it will work the way the external
array
code expects.
Done.
https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions.cc#newcode1865
src/hydrogen-instructions.cc:1865: switch (elements_kind()) {
On 2012/10/22 06:14:18, Sven Panne wrote:
DBC: Add a new function ElementsKind2String and use it in
PrintElementsKind and
here.
Good idea, thanks Sven. Now, the string will be different (instead of
"byte") it would be EXTERNAL_BYTE_ELEMENTS, but I guess that is okay
with us. I added an assert to make sure that down this branch we only
accept external_* elementkinds.
https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions.cc#newcode1948
src/hydrogen-instructions.cc:1948: // fast double HLoadKeyed.
On 2012/10/21 20:44:41, danno wrote:
Yes, this should be OK
Done.
https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions.cc#newcode2014
src/hydrogen-instructions.cc:2014: stream->Add("[");
On 2012/10/21 20:44:41, danno wrote:
Can share from above line...
Done.
https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions.cc#newcode2017
src/hydrogen-instructions.cc:2017: value()->PrintNameTo(stream);
On 2012/10/21 20:44:41, danno wrote:
...to line above between both external and non-external cases.
Done, also fixed duplicate code issue at line 2005.
https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions.cc#newcode2021
src/hydrogen-instructions.cc:2021: switch (elements_kind()) {
On 2012/10/22 06:14:18, Sven Panne wrote:
See my comment above...
Done.
https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):
https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions.h#newcode4345
src/hydrogen-instructions.h:4345: // Is this nessecary at all?
On 2012/10/21 20:44:41, danno wrote:
We should be able to treat all array accesses uniformly, including
optimizing
away bounds checks on external array, so this code shouldn't be
necessary. BTW:
You should make sure that array bounds check elimination works for
external
arrays after your changes.
Done.
https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions.h#newcode4357
src/hydrogen-instructions.h:4357: return is_external() ? true :
!RequiresHoleCheck();
On 2012/10/21 20:44:41, danno wrote:
RequiresHoleCheck should return false for external arrays, so
IsDeletable should
just return !RequiresHoleCheck()
Done.
https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions.h#newcode4497
src/hydrogen-instructions.h:4497: ElementsKind elements_kind, bool
is_external)
On 2012/10/21 20:44:41, danno wrote:
Same feedback here about is_external as in the load case... it should
be
computed from elements_kind
Done.
https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions.h#newcode4515
src/hydrogen-instructions.h:4515: // (<tagged> | <external>>[int32] =
<tagged> | double | int
On 2012/10/21 20:44:41, danno wrote:
nit: (<tagged> | <external>)[int32] = (<tagged> | double | int)
Done.
https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions.h#newcode4523
src/hydrogen-instructions.h:4523: ASSERT_EQ(index, 2);
On 2012/10/21 20:44:41, danno wrote:
Perhaps add a new predicate in elements-kind.h that detects all
floating/double
types?
e.g. create:
inline bool IsDoubleOrFloatElementsKind(ElementsKind elements_kind) {
return IsFastDoubleElementsKind(elements_kind) ||
elements_kind == EXTERNAL_DOUBLE_ELEMENTS ||
elements_kind == EXTERNAL_FLOAT_ELEMENTS;
}
then the rest of this function become:
if (IsDoubleOrFloatElementsKind()) return Representation::Double();
return is_external() ? Representation::Tagged() :
Representation::Integer32();
Wow, that was quite a knot, thanks!
Actually, having spent an hour tracking down a bug, I found it here :D.
Your proposal should say
return is_external() ? Representation::INTEGER32() :
Representation::TAGGED(); (swapped)
Damn, making me read instead of copying and pasting :D.
https://codereview.chromium.org/11238016/diff/2001/src/hydrogen.cc
File src/hydrogen.cc (right):
https://codereview.chromium.org/11238016/diff/2001/src/hydrogen.cc#newcode3749
src/hydrogen.cc:3749: // What should be done about that?
On 2012/10/21 20:44:41, danno wrote:
Why has the limitation changed?
Because elements_kind was encoded in 5 bits, but because now it needs to
hold higher-numbered ElementsKinds it needs 6 bits. I'm not really
seeing contracts in the code around any particular #-of-bits for the
index field. I'd be happy to make some tests that make this more
explicit. Any thoughts on that topic?
https://codereview.chromium.org/11238016/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
