Added comments.
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);
We need at least a BuildCheckHeapObject(receiver) before
AddLoadElements(receiver). Please add a test for this case.
https://codereview.chromium.org/57123002/diff/1270001/src/code-stubs-hydrogen.cc#newcode1349
src/code-stubs-hydrogen.cc:1349: KeyedLoadGenericElementStub* stub)
nit: Weird indentation
https://codereview.chromium.org/57123002/diff/1270001/src/code-stubs-hydrogen.cc#newcode1376
src/code-stubs-hydrogen.cc:1376: ElementsKind handle_as_kind) {
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).
https://codereview.chromium.org/57123002/diff/1270001/src/code-stubs-hydrogen.cc#newcode1391
src/code-stubs-hydrogen.cc:1391: if
(!IsExternalArrayElementsKind(handle_as_kind)) {
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.
https://codereview.chromium.org/57123002/diff/1270001/src/code-stubs-hydrogen.cc#newcode1426
src/code-stubs-hydrogen.cc:1426: BuildReceiverCheck(receiver,
bit_field_mask);
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?
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; }
This should be GENERIC instead of MEGAMORPHIC.
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);
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.
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);
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?)
https://codereview.chromium.org/57123002/diff/1270001/src/heap.cc#newcode7726
src/heap.cc:7726: return -index + map->inobject_properties() - 1;
Same as 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;
"value >= 1 << ..." is quite hard to understand. Could we extract this
to temporary variables that declare their meaning?
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(),
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?
https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen-instructions.cc#newcode3242
src/hydrogen-instructions.cc:3242:
HChange::cast(int_index)->InsertBefore(this);
So this change is actually (or better, will be) superflous.
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:
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? :)
https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen-instructions.h#newcode6792
src/hydrogen-instructions.h:6792: void IncreaseBaseOffset(uint32_t
base_offset) {
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)
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));
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.
https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode1465
src/hydrogen.cc:1465: Add<HLoadNamedField>(map,
HObjectAccess::ForMapBitField());
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.
https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode1471
src/hydrogen.cc:1471:
access_check.If<HCompareNumericAndBranch>(and_result,
Use DeoptimizeIf.
https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode1488
src/hydrogen.cc:1488: return AddUncasted<HShr>(index,
Add<HConstant>(String::kHashShift));
A bitfield decoding mechanism would be useful again.
https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode1493
src/hydrogen.cc:1493: HIfContinuation* join_continuation) {
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.)
https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode1510
src/hydrogen.cc:1510: IfBuilder not_string_or_name_if(this);
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.
https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode1527
src/hydrogen.cc:1527: name_if.If<HCompareNumericAndBranch>(
And here name_if means that it's not a name. That's also pretty
confusing.
https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode1541
src/hydrogen.cc:1541:
Add<HConstant>(Name::kContainsCachedArrayIndexMask));
Shouldn't kContainsCachedArrayIndex be on String rather than Name? Names
can't have cached array indices it seems to me.
https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode1550
src/hydrogen.cc:1550: Push(BuildHashToIndex(hash));
BuildHashToIndex could just be a bitfield decoder.
https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode1584
src/hydrogen.cc:1584: // not one of the global object types.
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.
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;
globAL
https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode1607
src/hydrogen.cc:1607: builder.CaptureContinuation(continuation);
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.
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);
nit: move * to HValue.
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/groups/opt_out.
