[turbofan]: Integrate basic type feedback for property accesses.

[turbofan]: Integrate basic type feedback for property accesses.

BUG=

Committed: https://crrev.com/78abf9d9d96cde33420028979b84178f1e774c56
Cr-Commit-Position: refs/heads/master@{#27470}

[titzer, 2015-03-19 16:56:19]

titzer@chromium.org changed reviewers:
+ mstarzinger@chromium.org, verwaest@chromium.org

[titzer, 2015-03-19 16:56:20]

First version, slightly wrong due to wrong deopt locations.

[Michael Starzinger, 2015-03-24 09:06:40]

https://codereview.chromium.org/1021713005/diff/20001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/1021713005/diff/20001/src/compiler.cc#newcode421
src/compiler.cc:421: } else if (FLAG_turbo_type_feedback) {
Can we please move this into the CompilationInfo constructor?

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/ast-graph-...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:1938: BuildNamedLoad(object, name, pair,
expr->AssignmentFeedbackId());
Shouldn't the property loads in the assignment use
property->PropertyFeedbackId() here?

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:1949: BuildKeyedLoad(object, key, pair,
expr->AssignmentFeedbackId());
Likewise.

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/ast-graph-...
File src/compiler/ast-graph-builder.h (right):

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.h:25: class JSTypeFeedbackTable;
nit: Please alpha-sort.

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.h:271: const VectorSlotPair& feedback,
TypeFeedbackId id);
suggestion: Would it make sense to make TypeFeedbackId::None() a default
argument for all four of these methods?

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/node-prope...
File src/compiler/node-properties.h (right):

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/node-prope...
src/compiler/node-properties.h:85: static void MergeControlToEnd(Graph* graph,
CommonOperatorBuilder* common,
Can we get a short comment explaining the intent of this method?

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/pipeline.cc
File src/compiler/pipeline.cc (right):

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/pipeline.c...
src/compiler/pipeline.cc:271: JSTypeFeedbackTable* js_type_feedback_;
nit: If the feedback table lives in this block, it should be set to NULL in
PipelineData::DeleteGraphZone as well.

[Michael Starzinger, 2015-03-24 09:25:30]

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/js-type-fe...
File src/compiler/js-type-feedback.cc (right):

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/js-type-fe...
src/compiler/js-type-feedback.cc:226: FieldAccess map_access = {kTaggedBase,
JSObject::kMapOffset,
Can we use AccessBuilder::ForMap here?

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/js-type-fe...
File src/compiler/js-type-feedback.h (right):

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/js-type-fe...
src/compiler/js-type-feedback.h:31: Node* Record(Node* node, TypeFeedbackId id);
nit: Can we make this return void instead?

[Toon Verwaest, 2015-03-24 09:42:39]

Some first comments. There seems to be lots of duplication between StoreNamed /
LoadNamed.

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/js-type-fe...
File src/compiler/js-type-feedback.cc (right):

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/js-type-fe...
src/compiler/js-type-feedback.cc:87: if (property_details.cell_type() ==
PropertyCellType::kConstant) {
This is not how constants are encoded. PropertyDetails have 3 partially distinct
encodings. Dictionary, global, and fast-mode objects. Dictionary and global are
mostly the same, minus that global also has PropertyCellType. Here we only care
about fast-mode objects though. For now you probably want to disallow everything
that's property_details.type() != DATA, which includes DATA_CONSTANT, and both
kinds of accessors.

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/js-type-fe...
src/compiler/js-type-feedback.cc:94: // TODO(turbofan): constants using code
dependencies.
Fast-mode-map-based constants don't use code dependencies. The map will change.

[titzer, 2015-03-24 10:49:32]

https://codereview.chromium.org/1021713005/diff/20001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/1021713005/diff/20001/src/compiler.cc#newcode421
src/compiler.cc:421: } else if (FLAG_turbo_type_feedback) {
On 2015/03/24 09:06:39, Michael Starzinger wrote:
> Can we please move this into the CompilationInfo constructor?

As discussed in person, we only want to enable type feedback for non-asm
functions at the moment.

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/ast-graph-...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:1938: BuildNamedLoad(object, name, pair,
expr->AssignmentFeedbackId());
On 2015/03/24 09:06:40, Michael Starzinger wrote:
> Shouldn't the property loads in the assignment use
> property->PropertyFeedbackId() here?

I cribbed from typing.cc which adds the type feedback to the AST for Crankshaft,
and it uses the AssignmentFeedbackId.

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:1949: BuildKeyedLoad(object, key, pair,
expr->AssignmentFeedbackId());
On 2015/03/24 09:06:40, Michael Starzinger wrote:
> Likewise.

Same as above.

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/ast-graph-...
File src/compiler/ast-graph-builder.h (right):

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.h:25: class JSTypeFeedbackTable;
On 2015/03/24 09:06:40, Michael Starzinger wrote:
> nit: Please alpha-sort.

Done.

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.h:271: const VectorSlotPair& feedback,
TypeFeedbackId id);
On 2015/03/24 09:06:40, Michael Starzinger wrote:
> suggestion: Would it make sense to make TypeFeedbackId::None() a default
> argument for all four of these methods?

I prefer the explicit usage of TypeFeedbackId::None(), since that marks
locations where we will want to add new id's in the future.

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/js-type-fe...
File src/compiler/js-type-feedback.cc (right):

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/js-type-fe...
src/compiler/js-type-feedback.cc:87: if (property_details.cell_type() ==
PropertyCellType::kConstant) {
On 2015/03/24 09:42:39, Toon Verwaest wrote:
> This is not how constants are encoded. PropertyDetails have 3 partially
distinct
> encodings. Dictionary, global, and fast-mode objects. Dictionary and global
are
> mostly the same, minus that global also has PropertyCellType. Here we only
care
> about fast-mode objects though. For now you probably want to disallow
everything
> that's property_details.type() != DATA, which includes DATA_CONSTANT, and both
> kinds of accessors.

So the condition should be "property_details.type() != DATA"?

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/js-type-fe...
src/compiler/js-type-feedback.cc:94: // TODO(turbofan): constants using code
dependencies.
On 2015/03/24 09:42:39, Toon Verwaest wrote:
> Fast-mode-map-based constants don't use code dependencies. The map will
change.

So I can just remove this condition?

The actual threading through of what constant to load needs to be thought out
into an abstractification.

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/js-type-fe...
File src/compiler/js-type-feedback.h (right):

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/js-type-fe...
src/compiler/js-type-feedback.h:31: Node* Record(Node* node, TypeFeedbackId id);
On 2015/03/24 09:25:29, Michael Starzinger wrote:
> nit: Can we make this return void instead?

Done.

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/node-prope...
File src/compiler/node-properties.h (right):

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/node-prope...
src/compiler/node-properties.h:85: static void MergeControlToEnd(Graph* graph,
CommonOperatorBuilder* common,
On 2015/03/24 09:06:40, Michael Starzinger wrote:
> Can we get a short comment explaining the intent of this method?

Done.

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/pipeline.cc
File src/compiler/pipeline.cc (right):

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/pipeline.c...
src/compiler/pipeline.cc:271: JSTypeFeedbackTable* js_type_feedback_;
On 2015/03/24 09:06:40, Michael Starzinger wrote:
> nit: If the feedback table lives in this block, it should be set to NULL in
> PipelineData::DeleteGraphZone as well.

Done.

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/verifier.cc
File src/compiler/verifier.cc (right):

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/verifier.c...
src/compiler/verifier.cc:622: /* TODO(titzer)
Haha, no one noticed this!

https://codereview.chromium.org/1021713005/diff/20001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/1021713005/diff/20001/src/flag-definitions.h#...
src/flag-definitions.h:407: DEFINE_IMPLICATION(turbo_type_feedback,
turbo_deoptimization)
Comments on this? Should type feedback imply deoptimization support? We could
still support fast path map checks and guarded inlining without deoptimizing,
but nothing that requires code dependencies.

[Michael Starzinger, 2015-03-24 12:11:18]

https://codereview.chromium.org/1021713005/diff/20001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/1021713005/diff/20001/src/compiler.cc#newcode421
src/compiler.cc:421: } else if (FLAG_turbo_type_feedback) {
On 2015/03/24 10:49:31, titzer wrote:
> On 2015/03/24 09:06:39, Michael Starzinger wrote:
> > Can we please move this into the CompilationInfo constructor?
> 
> As discussed in person, we only want to enable type feedback for non-asm
> functions at the moment.

Acknowledged.

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/ast-graph-...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:1938: BuildNamedLoad(object, name, pair,
expr->AssignmentFeedbackId());
On 2015/03/24 10:49:31, titzer wrote:
> On 2015/03/24 09:06:40, Michael Starzinger wrote:
> > Shouldn't the property loads in the assignment use
> > property->PropertyFeedbackId() here?
> 
> I cribbed from typing.cc which adds the type feedback to the AST for
Crankshaft,
> and it uses the AssignmentFeedbackId.

As discussed offline: In typing.cc the AstTyper::VisitAssignment recurses on
expr->target, which in turn will go into AstTyper::VisitProperty where
expr->target()->PropertyFeedbackId() is used.

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:1949: BuildKeyedLoad(object, key, pair,
expr->AssignmentFeedbackId());
On 2015/03/24 10:49:31, titzer wrote:
> On 2015/03/24 09:06:40, Michael Starzinger wrote:
> > Likewise.
> 
> Same as above.

Likewise.

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/ast-graph-...
File src/compiler/ast-graph-builder.h (right):

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.h:271: const VectorSlotPair& feedback,
TypeFeedbackId id);
On 2015/03/24 10:49:31, titzer wrote:
> On 2015/03/24 09:06:40, Michael Starzinger wrote:
> > suggestion: Would it make sense to make TypeFeedbackId::None() a default
> > argument for all four of these methods?
> 
> I prefer the explicit usage of TypeFeedbackId::None(), since that marks
> locations where we will want to add new id's in the future.

Acknowledged.

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/verifier.cc
File src/compiler/verifier.cc (right):

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/verifier.c...
src/compiler/verifier.cc:622: /* TODO(titzer)
On 2015/03/24 10:49:32, titzer wrote:
> Haha, no one noticed this!

Maeh, verifier, shmerifier. :)

https://codereview.chromium.org/1021713005/diff/20001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/1021713005/diff/20001/src/flag-definitions.h#...
src/flag-definitions.h:407: DEFINE_IMPLICATION(turbo_type_feedback,
turbo_deoptimization)
On 2015/03/24 10:49:32, titzer wrote:
> Comments on this? Should type feedback imply deoptimization support? We could
> still support fast path map checks and guarded inlining without deoptimizing,
> but nothing that requires code dependencies.

Agreed, it would be nice to be able to leverage dynamic feedback even without
deoptimization support. Not sure if we ever want to run in such a mode, but it
would be nice to know that we could. :)

[Toon Verwaest, 2015-03-24 12:56:12]

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/js-type-fe...
File src/compiler/js-type-feedback.cc (right):

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/js-type-fe...
src/compiler/js-type-feedback.cc:87: if (property_details.cell_type() ==
PropertyCellType::kConstant) {
On 2015/03/24 10:49:31, titzer wrote:
> On 2015/03/24 09:42:39, Toon Verwaest wrote:
> > This is not how constants are encoded. PropertyDetails have 3 partially
> distinct
> > encodings. Dictionary, global, and fast-mode objects. Dictionary and global
> are
> > mostly the same, minus that global also has PropertyCellType. Here we only
> care
> > about fast-mode objects though. For now you probably want to disallow
> everything
> > that's property_details.type() != DATA, which includes DATA_CONSTANT, and
both
> > kinds of accessors.
> 
> So the condition should be "property_details.type() != DATA"?

indeed

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/js-type-fe...
src/compiler/js-type-feedback.cc:94: // TODO(turbofan): constants using code
dependencies.
On 2015/03/24 10:49:32, titzer wrote:
> On 2015/03/24 09:42:39, Toon Verwaest wrote:
> > Fast-mode-map-based constants don't use code dependencies. The map will
> change.
> 
> So I can just remove this condition?
> 
> The actual threading through of what constant to load needs to be thought out
> into an abstractification.

Indeed. This condition makes no sense. You'll again need to do if
(property_details.type() != DATA) return false;

[titzer, 2015-03-24 14:52:06]

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/ast-graph-...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:1938: BuildNamedLoad(object, name, pair,
expr->AssignmentFeedbackId());
On 2015/03/24 12:11:18, Michael Starzinger wrote:
> On 2015/03/24 10:49:31, titzer wrote:
> > On 2015/03/24 09:06:40, Michael Starzinger wrote:
> > > Shouldn't the property loads in the assignment use
> > > property->PropertyFeedbackId() here?
> > 
> > I cribbed from typing.cc which adds the type feedback to the AST for
> Crankshaft,
> > and it uses the AssignmentFeedbackId.
> 
> As discussed offline: In typing.cc the AstTyper::VisitAssignment recurses on
> expr->target, which in turn will go into AstTyper::VisitProperty where
> expr->target()->PropertyFeedbackId() is used.

Done.

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:1949: BuildKeyedLoad(object, key, pair,
expr->AssignmentFeedbackId());
On 2015/03/24 12:11:17, Michael Starzinger wrote:
> On 2015/03/24 10:49:31, titzer wrote:
> > On 2015/03/24 09:06:40, Michael Starzinger wrote:
> > > Likewise.
> > 
> > Same as above.
> 
> Likewise.

Done.

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/js-type-fe...
File src/compiler/js-type-feedback.cc (right):

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/js-type-fe...
src/compiler/js-type-feedback.cc:87: if (property_details.cell_type() ==
PropertyCellType::kConstant) {
On 2015/03/24 12:56:12, Toon Verwaest wrote:
> On 2015/03/24 10:49:31, titzer wrote:
> > On 2015/03/24 09:42:39, Toon Verwaest wrote:
> > > This is not how constants are encoded. PropertyDetails have 3 partially
> > distinct
> > > encodings. Dictionary, global, and fast-mode objects. Dictionary and
global
> > are
> > > mostly the same, minus that global also has PropertyCellType. Here we only
> > care
> > > about fast-mode objects though. For now you probably want to disallow
> > everything
> > > that's property_details.type() != DATA, which includes DATA_CONSTANT, and
> both
> > > kinds of accessors.
> > 
> > So the condition should be "property_details.type() != DATA"?
> 
> indeed

Done.

https://codereview.chromium.org/1021713005/diff/20001/src/compiler/js-type-fe...
src/compiler/js-type-feedback.cc:94: // TODO(turbofan): constants using code
dependencies.
On 2015/03/24 12:56:12, Toon Verwaest wrote:
> On 2015/03/24 10:49:32, titzer wrote:
> > On 2015/03/24 09:42:39, Toon Verwaest wrote:
> > > Fast-mode-map-based constants don't use code dependencies. The map will
> > change.
> > 
> > So I can just remove this condition?
> > 
> > The actual threading through of what constant to load needs to be thought
out
> > into an abstractification.
> 
> Indeed. This condition makes no sense. You'll again need to do if
> (property_details.type() != DATA) return false;

Done.

https://codereview.chromium.org/1021713005/diff/20001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/1021713005/diff/20001/src/flag-definitions.h#...
src/flag-definitions.h:407: DEFINE_IMPLICATION(turbo_type_feedback,
turbo_deoptimization)
On 2015/03/24 12:11:18, Michael Starzinger wrote:
> On 2015/03/24 10:49:32, titzer wrote:
> > Comments on this? Should type feedback imply deoptimization support? We
could
> > still support fast path map checks and guarded inlining without
deoptimizing,
> > but nothing that requires code dependencies.
> 
> Agreed, it would be nice to be able to leverage dynamic feedback even without
> deoptimization support. Not sure if we ever want to run in such a mode, but it
> would be nice to know that we could. :)

I removed the implication and put a branch in js-type-feedback.cc

[Michael Starzinger, 2015-03-24 14:59:40]

LGTM from my end. I didn't look at the JSTypeFeedbackSpecializer logic, but I
believe that Toon has that one covered.

[Toon Verwaest, 2015-03-24 15:32:06]

JSTypeFeedbackSpecializer lgtm

[titzer, 2015-03-26 09:16:28]

The CQ bit was checked by titzer@chromium.org

[titzer, 2015-03-26 09:16:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org, verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/1021713005/#ps80001
(title: " ")

[commit-bot: I haz the power, 2015-03-26 09:16:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1021713005/80001

[commit-bot: I haz the power, 2015-03-26 09:38:16]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-26 09:38:35]

Patchset 5 (id:??) landed as
https://crrev.com/78abf9d9d96cde33420028979b84178f1e774c56
Cr-Commit-Position: refs/heads/master@{#27470}