Generate TypedArrayInitialize builtin in hydrogen.

Generate TypedArrayInitialize builtin in hydrogen.

R=danno@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=18059

[Dmitry Lomov (no reviews), 2013-11-11 17:56:50]

Not a final CL (most notably missing platform changes required to support
HAdd(kExternal, kInteger32)), but take a look if you have a chance.

[Dmitry Lomov (no reviews), 2013-11-12 11:53:33]

Ready for review.

[Dmitry Lomov (no reviews), 2013-11-15 18:35:19]

ping

[mvstanton, 2013-11-18 10:44:49]

Hi Dmitry, first comments. Very nice, thx,
--Michael

https://codereview.chromium.org/59023003/diff/110001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/59023003/diff/110001/src/hydrogen.cc#newcode8065
src/hydrogen.cc:8065: ASSERT(arguments->at(1)->node_type() ==
AstNode::kLiteral);
Could you use constants for these arguments with a meaning-based name?

https://codereview.chromium.org/59023003/diff/110001/src/hydrogen.cc#newcode8067
src/hydrogen.cc:8067: ASSERT(value->IsSmi());
The assert is unnecessary because the Smi::cast will do the same.

https://codereview.chromium.org/59023003/diff/110001/src/hydrogen.cc#newcode8105
src/hydrogen.cc:8105: Add<HConstant>(static_cast<int32_t>(0)));
Not something we have to do now, but it would be neat if we had a memset
instruction that could do this very efficiently. I can think of other places
we'd use it.

https://codereview.chromium.org/59023003/diff/110001/src/hydrogen.cc#newcode8128
src/hydrogen.cc:8128: 
nit: Remove newline.

https://codereview.chromium.org/59023003/diff/110001/src/hydrogen.cc#newcode8138
src/hydrogen.cc:8138: isolate()->heap()->GetPretenureMode(),
It would be good if you continue passing NOT_TENURED here, like the current
runtime call does. The reason is that for the allocation-site-based pretenuring
work, we've identified sites that make use of this global mode and are (rather
painfully) addressing them in turn. I don't want that set to grow larger for the
moment.

https://codereview.chromium.org/59023003/diff/110001/src/property-details.h
File src/property-details.h (right):

https://codereview.chromium.org/59023003/diff/110001/src/property-details.h#n...
src/property-details.h:136: if (kind_ == kNone && other.kind_ == kExternal)
return false;
How do you know these are the only cases you'll be asked about? (kExternal,
kNone), (kExternal, kExternal), (kNone, kExternal)

[Dmitry Lomov (no reviews), 2013-11-18 13:48:14]

Comments addressed, PTAL

https://codereview.chromium.org/59023003/diff/110001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/59023003/diff/110001/src/hydrogen.cc#newcode8065
src/hydrogen.cc:8065: ASSERT(arguments->at(1)->node_type() ==
AstNode::kLiteral);
On 2013/11/18 10:44:49, mvstanton wrote:
> Could you use constants for these arguments with a meaning-based name?

Done. Great suggestion, because it uncovered a bug :)

https://codereview.chromium.org/59023003/diff/110001/src/hydrogen.cc#newcode8067
src/hydrogen.cc:8067: ASSERT(value->IsSmi());
On 2013/11/18 10:44:49, mvstanton wrote:
> The assert is unnecessary because the Smi::cast will do the same.
I am paranoid lately.

https://codereview.chromium.org/59023003/diff/110001/src/hydrogen.cc#newcode8105
src/hydrogen.cc:8105: Add<HConstant>(static_cast<int32_t>(0)));
On 2013/11/18 10:44:49, mvstanton wrote:
> Not something we have to do now, but it would be neat if we had a memset
> instruction that could do this very efficiently. I can think of other places
> we'd use it. 
Agreed.

https://codereview.chromium.org/59023003/diff/110001/src/hydrogen.cc#newcode8128
src/hydrogen.cc:8128: 
On 2013/11/18 10:44:49, mvstanton wrote:
> nit: Remove newline.

Done.

https://codereview.chromium.org/59023003/diff/110001/src/hydrogen.cc#newcode8138
src/hydrogen.cc:8138: isolate()->heap()->GetPretenureMode(),
On 2013/11/18 10:44:49, mvstanton wrote:
> It would be good if you continue passing NOT_TENURED here, like the current
> runtime call does. The reason is that for the allocation-site-based
pretenuring
> work, we've identified sites that make use of this global mode and are (rather
> painfully) addressing them in turn. I don't want that set to grow larger for
the
> moment.

Done.

https://codereview.chromium.org/59023003/diff/110001/src/property-details.h
File src/property-details.h (right):

https://codereview.chromium.org/59023003/diff/110001/src/property-details.h#n...
src/property-details.h:136: if (kind_ == kNone && other.kind_ == kExternal)
return false;
On 2013/11/18 10:44:49, mvstanton wrote:
> How do you know these are the only cases you'll be asked about? (kExternal,
> kNone), (kExternal, kExternal), (kNone, kExternal)

Representation is a lattice where kNone is the bottom element and kExternal is
uncomparable with anything else. We assert on uncomparable.

[danno, 2013-11-18 14:26:16]

https://codereview.chromium.org/59023003/diff/110001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/59023003/diff/110001/src/hydrogen-instruction...
src/hydrogen-instructions.h:4627: static HAdd* NewExternalPointerOffset(
This doesn't really work well the the Add<> templates. I think better would be a
5th parameter to new of type Representation that is by default None(), but when
it's something else it sets the representation of the LHS and the observed
output representation. That way, it's still possible to use the Add<> templates
with an additional parameter.

https://codereview.chromium.org/59023003/diff/110001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/59023003/diff/110001/src/hydrogen.cc#newcode8161
src/hydrogen.cc:8161: external_pointer->ClearFlag(HValue::kCanOverflow);
This should be folded into the constructor of the Add based on the
representation, probably.

https://codereview.chromium.org/59023003/diff/110001/src/hydrogen.cc#newcode8176
src/hydrogen.cc:8176: byte_offset_smi.Else();
Perhaps use the {} indenting style used in code-stubs-hydrogen to help parse the
IfBuilder control flow?

https://codereview.chromium.org/59023003/diff/180001/src/property-details.h
File src/property-details.h (right):

https://codereview.chromium.org/59023003/diff/180001/src/property-details.h#n...
src/property-details.h:134: if (kind_ == kExternal && other.kind_ == kNone)
return true;
Can you please update the test-representation.cc test case to fully cover your
changes (and perhaps kExternal in general)?

[Dmitry Lomov (no reviews), 2013-11-18 16:43:53]

Comments addressed, please take a look.

I have rebased and refactored to avoid code duplication with
VisitDataViewInitialize - sorry for complicating things, but I think that makes
code better overall.

https://codereview.chromium.org/59023003/diff/110001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/59023003/diff/110001/src/hydrogen-instruction...
src/hydrogen-instructions.h:4627: static HAdd* NewExternalPointerOffset(
On 2013/11/18 14:26:17, danno wrote:
> This doesn't really work well the the Add<> templates. I think better would be
a
> 5th parameter to new of type Representation that is by default None(), but
when
> it's something else it sets the representation of the LHS and the observed
> output representation. That way, it's still possible to use the Add<>
templates
> with an additional parameter.

Done, even though Add<> does not work with HAdd even now, AFAIU.

https://codereview.chromium.org/59023003/diff/110001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/59023003/diff/110001/src/hydrogen.cc#newcode8161
src/hydrogen.cc:8161: external_pointer->ClearFlag(HValue::kCanOverflow);
On 2013/11/18 14:26:17, danno wrote:
> This should be folded into the constructor of the Add based on the
> representation, probably.

No, because it is in this particular case (in TypedArrayinitialize builtin) we
validate that (backing_store +byte_offset) cannot overflow. If we didn't do
that, it wouldn't be correct to assume no overflow here (and cause *interesting*
bugs!)

https://codereview.chromium.org/59023003/diff/110001/src/hydrogen.cc#newcode8176
src/hydrogen.cc:8176: byte_offset_smi.Else();
On 2013/11/18 14:26:17, danno wrote:
> Perhaps use the {} indenting style used in code-stubs-hydrogen to help parse
the
> IfBuilder control flow?

Done.

https://codereview.chromium.org/59023003/diff/180001/src/property-details.h
File src/property-details.h (right):

https://codereview.chromium.org/59023003/diff/180001/src/property-details.h#n...
src/property-details.h:134: if (kind_ == kExternal && other.kind_ == kNone)
return true;
On 2013/11/18 14:26:17, danno wrote:
> Can you please update the test-representation.cc test case to fully cover your
> changes (and perhaps kExternal in general)?

Done.

[Dmitry Lomov (no reviews), 2013-11-19 09:42:03]

Updated patch with a "better" implementation of HAdd::New. I still have
reservations.

https://codereview.chromium.org/59023003/diff/260001/src/hydrogen-instruction...
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/59023003/diff/260001/src/hydrogen-instruction...
src/hydrogen-instructions.cc:3788: if (!forced_representation.IsNone()) {
I am not entirely convinced this is a good change. In my original patch, I
explicitly created an (external + offset) => external instruction, which is the
only case we handle downstream. Here it appears that we can handle a (r + smth)
=> r for all r, which is not true. wdyt?

[danno, 2013-11-20 09:45:24]

https://codereview.chromium.org/59023003/diff/260001/src/hydrogen-instruction...
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/59023003/diff/260001/src/hydrogen-instruction...
src/hydrogen-instructions.cc:3788: if (!forced_representation.IsNone()) {
On 2013/11/19 09:42:04, Dmitry Lomov (chromium) wrote:
> I am not entirely convinced this is a good change. In my original patch, I
> explicitly created an (external + offset) => external instruction, which is
the
> only case we handle downstream. Here it appears that we can handle a (r +
smth)
> => r for all r, which is not true. wdyt?

I see your point, but I also really don't like the idea of two instructions. It
seems the Add should "automagically" do the right thing when one of it's inputs
is an external pointer, since that's the special case you need to handle. i.e.
why do you need to set the observed input representation in the first place?
Shouldn't the instruction just do the right thing when this input is External?
If not, there seems to be some missing machinery somewhere else.

Alternatively, since there are only two additional calls that are needed in this
path, can it be done separately from the constructor? i.e. remove the input to
the constructor and have a "MarkAsExternalPointerAdd" method?

In any case, let's discuss in person.

[Dmitry Lomov (no reviews), 2013-11-20 10:43:26]

On 2013/11/20 09:45:24, danno wrote:
>
https://codereview.chromium.org/59023003/diff/260001/src/hydrogen-instruction...
> File src/hydrogen-instructions.cc (right):
> 
>
https://codereview.chromium.org/59023003/diff/260001/src/hydrogen-instruction...
> src/hydrogen-instructions.cc:3788: if (!forced_representation.IsNone()) {
> On 2013/11/19 09:42:04, Dmitry Lomov (chromium) wrote:
> > I am not entirely convinced this is a good change. In my original patch, I
> > explicitly created an (external + offset) => external instruction, which is
> the
> > only case we handle downstream. Here it appears that we can handle a (r +
> smth)
> > => r for all r, which is not true. wdyt?
> 
> I see your point, but I also really don't like the idea of two instructions.
It
> seems the Add should "automagically" do the right thing when one of it's
inputs
> is an external pointer, since that's the special case you need to handle. i.e.
> why do you need to set the observed input representation in the first place?
> Shouldn't the instruction just do the right thing when this input is External?
> If not, there seems to be some missing machinery somewhere else.
> 
> Alternatively, since there are only two additional calls that are needed in
this
> path, can it be done separately from the constructor? i.e. remove the input to
> the constructor and have a "MarkAsExternalPointerAdd" method?
> 
> In any case, let's discuss in person.

Turns out, this change has been masquerading some other, since fixed, bug in my
changes.
Changes to HAdd::RepresentationsFromInputs and HAdd::RequiredInputRepresentation
are quite enough.

[danno, 2013-11-20 13:50:50]

https://codereview.chromium.org/59023003/diff/380001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/59023003/diff/380001/src/hydrogen-instruction...
src/hydrogen-instructions.h:4751: static HInstruction* New(
nit: remove this whitespace change.

https://codereview.chromium.org/59023003/diff/380001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/59023003/diff/380001/src/hydrogen.cc#newcode8060
src/hydrogen.cc:8060: HGraphBuilder* graph,
this should be called "builder". Consider moving this method to the
HGraphBuilder super class, that avoids having to use graph-> everywhere.

https://codereview.chromium.org/59023003/diff/380001/src/hydrogen.cc#newcode8067
src/hydrogen.cc:8067: offset < ViewClass::kSizeWithInternalFields;
nit: alignment

https://codereview.chromium.org/59023003/diff/380001/src/hydrogen.cc#newcode8087
src/hydrogen.cc:8087: JSArrayBuffer::kWeakFirstViewOffset)));
nit: 4 char indent. Perhaps move the object access into a local.

https://codereview.chromium.org/59023003/diff/380001/src/hydrogen.cc#newcode8136
src/hydrogen.cc:8136:
static_cast<Literal*>(arguments->at(kArrayIdArg))->value();
4-char indent

https://codereview.chromium.org/59023003/diff/380001/src/hydrogen.cc#newcode8197
src/hydrogen.cc:8197: HObjectAccess::ForJSObjectOffset(
nit: 4 char indent

https://codereview.chromium.org/59023003/diff/380001/src/hydrogen.cc#newcode8198
src/hydrogen.cc:8198: JSArrayBuffer::kBackingStoreOffset,
Representation::External()));
Same here. Maybe move the access into a local variable? Makes the line shorter.

https://codereview.chromium.org/59023003/diff/380001/src/hydrogen.cc#newcode8206
src/hydrogen.cc:8206: zone(), graph()->GetInvalidContext(), backing_store,
byte_offset);
You should be able to use Add<> now, shouldn't you? Add returns the typed
instruction pointer, so you can call ClearFlag after-the-fact.

https://codereview.chromium.org/59023003/diff/380001/src/hydrogen.cc#newcode8215
src/hydrogen.cc:8215: ExternalArray::kExternalPointerOffset,
Representation::External()),
nit: 4 char indent?

https://codereview.chromium.org/59023003/diff/380001/src/hydrogen.cc#newcode8235
src/hydrogen.cc:8235: byte_offset_smi.End();
Shouldn't this be after the } below? It should always be called.

[Dmitry Lomov (no reviews), 2013-11-20 17:38:56]

Comments addressed + take a look at a change in lithium-codegen-x64.cc

https://codereview.chromium.org/59023003/diff/380001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/59023003/diff/380001/src/hydrogen-instruction...
src/hydrogen-instructions.h:4751: static HInstruction* New(
On 2013/11/20 13:50:50, danno wrote:
> nit: remove this whitespace change.

Done.

https://codereview.chromium.org/59023003/diff/380001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/59023003/diff/380001/src/hydrogen.cc#newcode8060
src/hydrogen.cc:8060: HGraphBuilder* graph,
On 2013/11/20 13:50:50, danno wrote:
> this should be called "builder". Consider moving this method to the
> HGraphBuilder super class, that avoids having to use graph-> everywhere.

Done.

https://codereview.chromium.org/59023003/diff/380001/src/hydrogen.cc#newcode8067
src/hydrogen.cc:8067: offset < ViewClass::kSizeWithInternalFields;
On 2013/11/20 13:50:50, danno wrote:
> nit: alignment

Done.

https://codereview.chromium.org/59023003/diff/380001/src/hydrogen.cc#newcode8087
src/hydrogen.cc:8087: JSArrayBuffer::kWeakFirstViewOffset)));
On 2013/11/20 13:50:50, danno wrote:
> nit: 4 char indent. Perhaps move the object access into a local.

Done.

https://codereview.chromium.org/59023003/diff/380001/src/hydrogen.cc#newcode8136
src/hydrogen.cc:8136:
static_cast<Literal*>(arguments->at(kArrayIdArg))->value();
On 2013/11/20 13:50:50, danno wrote:
> 4-char indent

Done.

https://codereview.chromium.org/59023003/diff/380001/src/hydrogen.cc#newcode8197
src/hydrogen.cc:8197: HObjectAccess::ForJSObjectOffset(
On 2013/11/20 13:50:50, danno wrote:
> nit: 4 char indent

Done.

https://codereview.chromium.org/59023003/diff/380001/src/hydrogen.cc#newcode8198
src/hydrogen.cc:8198: JSArrayBuffer::kBackingStoreOffset,
Representation::External()));
On 2013/11/20 13:50:50, danno wrote:
> Same here. Maybe move the access into a local variable? Makes the line
shorter.

Done.

https://codereview.chromium.org/59023003/diff/380001/src/hydrogen.cc#newcode8206
src/hydrogen.cc:8206: zone(), graph()->GetInvalidContext(), backing_store,
byte_offset);
On 2013/11/20 13:50:50, danno wrote:
> You should be able to use Add<> now, shouldn't you? Add returns the typed
> instruction pointer, so you can call ClearFlag after-the-fact.

Done.

https://codereview.chromium.org/59023003/diff/380001/src/hydrogen.cc#newcode8215
src/hydrogen.cc:8215: ExternalArray::kExternalPointerOffset,
Representation::External()),
On 2013/11/20 13:50:50, danno wrote:
> nit: 4 char indent?

Done.

https://codereview.chromium.org/59023003/diff/380001/src/hydrogen.cc#newcode8235
src/hydrogen.cc:8235: byte_offset_smi.End();
On 2013/11/20 13:50:50, danno wrote:
> Shouldn't this be after the } below? It should always be called.

Done.

[danno, 2013-11-21 20:44:32]

Getting very close...

https://codereview.chromium.org/59023003/diff/510001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/59023003/diff/510001/src/hydrogen.cc#newcode8176
src/hydrogen.cc:8176:
HObjectAccess::ForJSObjectOffset(JSTypedArray::kLengthOffset),
Here an elsewhere, does it perhaps make sense to wrap these HObjectAccess
builders in utility functions? That's what we've done elsewhere, it provides
another level of indirection, e.g.:

HObjectAccess::ForJSTypedArrayLength()

https://codereview.chromium.org/59023003/diff/510001/src/hydrogen.cc#newcode8180
src/hydrogen.cc:8180: Add<HAllocate>(
nit: 4 char indent

https://codereview.chromium.org/59023003/diff/510001/src/hydrogen.cc#newcode8188
src/hydrogen.cc:8188: 
nit: Group the load/store pairs and their preparation that belong together, e.g.
remove this line

https://codereview.chromium.org/59023003/diff/510001/src/hydrogen.cc#newcode8191
src/hydrogen.cc:8191: Add<HConstant>(external_array_map));
nit: And insert one after this line

https://codereview.chromium.org/59023003/diff/510001/src/hydrogen.cc#newcode8195
src/hydrogen.cc:8195: 
nit: remove this line

https://codereview.chromium.org/59023003/diff/510001/src/hydrogen.cc#newcode8215
src/hydrogen.cc:8215:
HObjectAccess::ForJSObjectOffset(ExternalArray::kLengthOffset),
I think it's safe to use HObjectAccoess::ForFixedArrayLength() here, since
ExternalArray derives from FixedArrayBase, and ForFixedArrayLength is valid for
all subclasses of it.

[Dmitry Lomov (no reviews), 2013-11-22 12:40:26]

PTAL. Comments addressed, with reservations :)

https://codereview.chromium.org/59023003/diff/510001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/59023003/diff/510001/src/hydrogen.cc#newcode8176
src/hydrogen.cc:8176:
HObjectAccess::ForJSObjectOffset(JSTypedArray::kLengthOffset),
On 2013/11/21 20:44:35, danno wrote:
> Here an elsewhere, does it perhaps make sense to wrap these HObjectAccess
> builders in utility functions? That's what we've done elsewhere, it provides
> another level of indirection, e.g.:
> 
> HObjectAccess::ForJSTypedArrayLength()

Done, albeit reluctantly. I see the value of encapsulating field information
(esp. representations), however I am not too happy to see one huge sheet of all
fields of all objects in HObjectAccess class.

https://codereview.chromium.org/59023003/diff/510001/src/hydrogen.cc#newcode8180
src/hydrogen.cc:8180: Add<HAllocate>(
On 2013/11/21 20:44:35, danno wrote:
> nit: 4 char indent

Done.

https://codereview.chromium.org/59023003/diff/510001/src/hydrogen.cc#newcode8188
src/hydrogen.cc:8188: 
On 2013/11/21 20:44:35, danno wrote:
> nit: Group the load/store pairs and their preparation that belong together,
e.g.
> remove this line

Done.

https://codereview.chromium.org/59023003/diff/510001/src/hydrogen.cc#newcode8191
src/hydrogen.cc:8191: Add<HConstant>(external_array_map));
On 2013/11/21 20:44:35, danno wrote:
> nit: And insert one after this line

Done.

https://codereview.chromium.org/59023003/diff/510001/src/hydrogen.cc#newcode8195
src/hydrogen.cc:8195: 
On 2013/11/21 20:44:35, danno wrote:
> nit: remove this line

Done.

[danno, 2013-11-25 11:44:12]

lgtm with nits

https://codereview.chromium.org/59023003/diff/550001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/59023003/diff/550001/src/hydrogen-instruction...
src/hydrogen-instructions.h:6107: 
nit: perhaps only one line between functions

https://codereview.chromium.org/59023003/diff/550001/src/hydrogen-instruction...
src/hydrogen-instructions.h:6130: void PrintTo(StringStream* stream);
nit: perhaps one line above?

[Dmitry Lomov (no reviews), 2013-11-25 14:42:00]

Committed patchset #11 manually as r18059 (presubmit successful).