Finally, after 7 months.... please take another look!
https://codereview.chromium.org/57123002/diff/1270001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):
https://codereview.chromium.org/57123002/diff/1270001/src/code-stubs-hydrogen.cc#newcode1331
src/code-stubs-hydrogen.cc:1331: HValue* elements =
AddLoadElements(receiver);
On 2013/12/04 17:29:26, Toon Verwaest wrote:
We need at least a BuildCheckHeapObject(receiver) before
AddLoadElements(receiver). Please add a test for this case.
Why is that needed? We already did a map check in the IC before we got
here that guaranteed that it's a JSObject with dictionary element kinds.
I wrote a test case for this an it works as expected (am I missing
something?)
https://codereview.chromium.org/57123002/diff/1270001/src/code-stubs-hydrogen.cc#newcode1349
src/code-stubs-hydrogen.cc:1349: KeyedLoadGenericElementStub* stub)
On 2013/12/04 17:29:26, Toon Verwaest wrote:
nit: Weird indentation
Done.
https://codereview.chromium.org/57123002/diff/1270001/src/code-stubs-hydrogen.cc#newcode1376
src/code-stubs-hydrogen.cc:1376: ElementsKind handle_as_kind) {
On 2013/12/04 17:29:26, Toon Verwaest wrote:
In isolation it's very hard to see what this method tries to
accomplish. It's
very much tied to the way we efficiently check whether we can load a
value from
the current array using a certain operation. At least add a comment to
explain
this. (Essentially that the range [0, start-kind[ is already checked,
and that
that's why we now implicitly check [start-kind, next_kind[ using
bit_field2 <
next_kind).
I'd actually prefer to not pass in next_kind, but rather use LE on
handle_as_kind. (We'd probably have to ASSERT that handle_as_kind is
next_kind -
1).
Done.
https://codereview.chromium.org/57123002/diff/1270001/src/code-stubs-hydrogen.cc#newcode1391
src/code-stubs-hydrogen.cc:1391: if
(!IsExternalArrayElementsKind(handle_as_kind)) {
On 2013/12/04 17:29:26, Toon Verwaest wrote:
It's hard to understand what kind of graph is created in case you have
external
array elements. I'd prefer if you'd just make this a top-level
if-test, rather
than selectively changing sub-instructions.
Done.
https://codereview.chromium.org/57123002/diff/1270001/src/code-stubs-hydrogen.cc#newcode1426
src/code-stubs-hydrogen.cc:1426: BuildReceiverCheck(receiver,
bit_field_mask);
On 2013/12/04 17:29:26, Toon Verwaest wrote:
Can we pull these 2 checks out of the continuation? We do it in both
branches.
Or is there a reason it needs to be done after BuildKeyedIndexCheck?
Also, will the checks IsAccessCheckNeeded / HasIndexedInterceptor ever
change in
other places? If not, what about moving that into BuildReceiverCheck?
And maybe
renaming "BuildReceiverCheck" to something that indicates that we're
checking
whether the receiver is a "plain old javascript object" with safe
access
semantics?
No, these can't be moved since they are different checks. This one here
checks to see if the object has indexed interceptors and the other path
checks for named interceptors.
https://codereview.chromium.org/57123002/diff/1270001/src/code-stubs.h
File src/code-stubs.h (right):
https://codereview.chromium.org/57123002/diff/1270001/src/code-stubs.h#newcode1886
src/code-stubs.h:1886: virtual InlineCacheState GetICState() { return
MEGAMORPHIC; }
On 2013/12/04 17:29:26, Toon Verwaest wrote:
This should be GENERIC instead of MEGAMORPHIC.
Done.
https://codereview.chromium.org/57123002/diff/1270001/src/elements-kind.cc
File src/elements-kind.cc (right):
https://codereview.chromium.org/57123002/diff/1270001/src/elements-kind.cc#newcode70
src/elements-kind.cc:70: ? 0 : (FixedArray::kHeaderSize - kSmiTagSize);
On 2013/12/04 17:29:26, Toon Verwaest wrote:
Shouldn't that be kHeapObjectTag rather than kSmiTagSize?
Also, this relies on FixedArray::kHeaderSize being equal to
FixedDoubleArray::kHeaderSize. Please add an ASSERT to that effect.
Done.
https://codereview.chromium.org/57123002/diff/1270001/src/heap.cc
File src/heap.cc (right):
https://codereview.chromium.org/57123002/diff/1270001/src/heap.cc#newcode7715
src/heap.cc:7715: return -(index - map->inobject_properties() + 1);
On 2013/12/04 17:29:26, Toon Verwaest wrote:
Seems like this should be combined with the code in
PropertyIndex::translate/is_inobject. (Why is the index here and int
rather than
a PropertyIndex object?)
Well, essentially because they are are not the same as the index
returned from PropertyIndex.translate(), which returns negative values
for in object properties and positive values for out-of-object. Here, we
need exactly the opposite to work out-of-the-box with HLoadIndexField,
which doesn't have a map for the size of the object and therefore can
use negative values to do the math from the end of the object
dynamically.
https://codereview.chromium.org/57123002/diff/1270001/src/heap.cc#newcode7726
src/heap.cc:7726: return -index + map->inobject_properties() - 1;
On 2013/12/04 17:29:26, Toon Verwaest wrote:
Same as above.
See my comment above.
https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen-dehoist.cc
File src/hydrogen-dehoist.cc (right):
https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen-dehoist.cc#newcode56
src/hydrogen-dehoist.cc:56: if (value >= 1 <<
array_operation->MaxBaseOffsetBits() || value < 0) return;
On 2013/12/04 17:29:26, Toon Verwaest wrote:
"value >= 1 << ..." is quite hard to understand. Could we extract this
to
temporary variables that declare their meaning?
Done.
https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):
https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen-instructions.cc#newcode3235
src/hydrogen-instructions.cc:3235: key_load->elements_kind(),
On 2013/12/04 17:29:26, Toon Verwaest wrote:
Why is this the same elements kind as the array of names? Wouldn't it
make more
sense to know that the index cache is eg a FAST_SMI_ELEMENTS array, so
that we
can avoid smi-checks? (For the future), even better, why don't we make
the cache
external ints or so, so the HLoadFieldByIndex automatically gets the
int without
any conversions?
That's beyond the scope of this change, which itself is no longer
necessary anyway after unrelated changes to HLoadFieldByIndex, the
for-in case remains as it always has been.
https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen-instructions.cc#newcode3242
src/hydrogen-instructions.cc:3242:
HChange::cast(int_index)->InsertBefore(this);
On 2013/12/04 17:29:26, Toon Verwaest wrote:
So this change is actually (or better, will be) superflous.
This change is no longer necessary, after unrelated changes to
HLoadFieldByIndex, the for-in case remains as it always has been.
https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):
https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen-instructions.h#newcode6361
src/hydrogen-instructions.h:6361:
On 2013/12/04 17:29:26, Toon Verwaest wrote:
super nit: seems like we're pretty inconsistent in single or double
newlines
between declarations in this patch of code. Can we just decide on
something? :)
Well, a few weeks ago we decided that we would just defer to
clang-format and be done with it. I still think that's the right thing
to do, I'll follow-up with Jochen to see how far he got with that.
https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen-instructions.h#newcode6792
src/hydrogen-instructions.h:6792: void IncreaseBaseOffset(uint32_t
base_offset) {
On 2013/12/04 17:29:26, Toon Verwaest wrote:
Could you add a comment indicating to what you are adding here
(constant-offset-from-key * kpointersize)?
And have a comment somewhere that indicates what the base-offset is
made up off?
(header + constant-offset-from-key * kpointersize)
Done.
https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc
File src/hydrogen.cc (right):
https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode1237
src/hydrogen.cc:1237: bit_field2,
Add<HConstant>(Map::kElementsKindShift));
On 2013/12/04 17:29:26, Toon Verwaest wrote:
It seems like ElementsKind should really be a BitField of bitfield2.
And then
perhaps we should have a function to read a BitField from an HValue;
rather than
manually shifting and masking.
Done.
https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode1465
src/hydrogen.cc:1465: Add<HLoadNamedField>(map,
HObjectAccess::ForMapBitField());
On 2013/12/04 17:29:26, Toon Verwaest wrote:
ForMapBitField2 I presume? Is this tested?
Seems to me like we should apply the same optimization here as above
by swapping
the order of the bitfields. That would at least unify the code.
Fields swapped, it makes this code much simpler.
https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode1471
src/hydrogen.cc:1471:
access_check.If<HCompareNumericAndBranch>(and_result,
On 2013/12/04 17:29:26, Toon Verwaest wrote:
Use DeoptimizeIf.
Done.
https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode1488
src/hydrogen.cc:1488: return AddUncasted<HShr>(index,
Add<HConstant>(String::kHashShift));
On 2013/12/04 17:29:26, Toon Verwaest wrote:
A bitfield decoding mechanism would be useful again.
Done.
https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode1493
src/hydrogen.cc:1493: HIfContinuation* join_continuation) {
On 2013/12/04 17:29:26, Toon Verwaest wrote:
It took me a while to figure out why the branches below are ordered
the way they
are. Please add a comment here indicating that join_continuation is
basically
the continuation for if-true->smi, if-false->something-else; and that
all
branches below need to match up to be able to join this continuation.
(Hence my confused comments below.)
Done.
https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode1510
src/hydrogen.cc:1510: IfBuilder not_string_or_name_if(this);
On 2013/12/04 17:29:26, Toon Verwaest wrote:
I'd swap the condition (use string_or_name rather than
not_string_or_name). The
double negation in the else branch is a bit confusing.
Ordering needs to stay the same due to continuation join.
https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode1527
src/hydrogen.cc:1527: name_if.If<HCompareNumericAndBranch>(
On 2013/12/04 17:29:26, Toon Verwaest wrote:
And here name_if means that it's not a name. That's also pretty
confusing.
Ordering needs to stay the same due to continuation join.
https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode1541
src/hydrogen.cc:1541:
Add<HConstant>(Name::kContainsCachedArrayIndexMask));
On 2013/12/04 17:29:26, Toon Verwaest wrote:
Shouldn't kContainsCachedArrayIndex be on String rather than Name?
Names can't
have cached array indices it seems to me.
Done.
https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode1550
src/hydrogen.cc:1550: Push(BuildHashToIndex(hash));
On 2013/12/04 17:29:26, Toon Verwaest wrote:
BuildHashToIndex could just be a bitfield decoder.
Done.
https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode1584
src/hydrogen.cc:1584: // not one of the global object types.
On 2013/12/04 17:29:26, Toon Verwaest wrote:
This comment seems to say the opposite of the name of the method and
the actual
body. The code (and name) guarantees that the receiver *is* a global
object, not
that it isn't.
Done.
https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode1592
src/hydrogen.cc:1592: int globla_type_count = JS_GLOBAL_PROXY_TYPE -
JS_GLOBAL_OBJECT_TYPE;
On 2013/12/04 17:29:26, Toon Verwaest wrote:
globAL
Done.
https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode1607
src/hydrogen.cc:1607: builder.CaptureContinuation(continuation);
On 2013/12/04 17:29:26, Toon Verwaest wrote:
I wonder if the resulting code may be smaller / faster if you'd read
out the
"is_dictionary_map" from the Map. That would avoid loading the
HashTableMap.
This is a copy of what the hand-written stub did IIRC. May I wait to try
optimizations in a separate CL?
https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.h
File src/hydrogen.h (right):
https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.h#newcode1295
src/hydrogen.h:1295: HValue* BuildHashToIndex(HValue *hash);
On 2013/12/04 17:29:26, Toon Verwaest wrote:
nit: move * to HValue.
Done.
https://codereview.chromium.org/57123002/
--
--
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.
