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-builder.cc
File src/compiler/ast-graph-builder.cc (right):
https://codereview.chromium.org/1021713005/diff/20001/src/compiler/ast-graph-builder.cc#newcode1938
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-builder.cc#newcode1949
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-builder.h
File src/compiler/ast-graph-builder.h (right):
https://codereview.chromium.org/1021713005/diff/20001/src/compiler/ast-graph-builder.h#newcode25
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-builder.h#newcode271
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-feedback.cc
File src/compiler/js-type-feedback.cc (right):
https://codereview.chromium.org/1021713005/diff/20001/src/compiler/js-type-feedback.cc#newcode87
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-feedback.cc#newcode94
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-feedback.h
File src/compiler/js-type-feedback.h (right):
https://codereview.chromium.org/1021713005/diff/20001/src/compiler/js-type-feedback.h#newcode31
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-properties.h
File src/compiler/node-properties.h (right):
https://codereview.chromium.org/1021713005/diff/20001/src/compiler/node-properties.h#newcode85
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.cc#newcode271
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.cc#newcode622
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#newcode407
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.
https://codereview.chromium.org/1021713005/
--
--
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.
