Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1149133005:
[es6] Add TF support for super.property (Closed)
|
Created:
5 years, 6 months ago by arv (Not doing code reviews) Modified:
5 years, 6 months ago CC:
v8-dev Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[es6] Add TF support for super.property
Currently does super.prop (load) and super.method() (call). Like full
codegen it uses runtime calls to load the property value.
BUG=v8:3330
LOG=N
R=mstarzinger@chromium.org, dslomov@chromium.org
Committed: https://crrev.com/06c706d84c04fc2e31e993518df9dcefb7543a2d
Cr-Commit-Position: refs/heads/master@{#28815}
Patch Set 1 : #
Total comments: 8
Messages
Total messages: 24 (10 generated)
Patchset #1 (id:1) has been deleted
arv@chromium.org changed reviewers: + mstarzinger@chromium.org
Hi Michi, I started hacking on this. My plan is to do: 1. super.prop using runtime calls 2. super() (construct) also with runtime calls 3. ??? 4. Profit It is not clear to me at the moment how we want to get rid of the runtime calls. My immediate goal is to make TF at least pass all the test. At this point I am a bit hazy about the FrameStateBeforeAndAfter and the last arg to FrameStateBeforeAndAfter::AddToNode. Does this need to be in sync with the full codegen? Not sure how it relates etc.
arv@chromium.org changed reviewers: + dslomov@chromium.org
Ready for review
self review
PTAL This is now complete and ready for review.
Update to use this_funtion_var instead of home_object_var
arv@chromium.org changed reviewers: + adamk@chromium.org
PTAL This is blocking my work on eval super
On 2015/06/04 17:15:06, arv wrote: > PTAL > > This is blocking my work on eval super I unblocked it... Take your time.
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
LGTM. https://codereview.chromium.org/1149133005/diff/100001/src/compiler/ast-graph... File src/compiler/ast-graph-builder.cc (right): https://codereview.chromium.org/1149133005/diff/100001/src/compiler/ast-graph... src/compiler/ast-graph-builder.cc:2313: if (!property->IsSuperAccess()) { Could this be modeled as a switch over the LhsKind as well? If so, can we leave a TODO here in that regard? https://codereview.chromium.org/1149133005/diff/100001/src/compiler/ast-graph... src/compiler/ast-graph-builder.cc:3461: return Record(js_type_feedback_, value, feedback.slot()); Just for clarification: My understanding is that we don't gather type-feedback for any of these super loads and stores yet. Correct? I am still fine with recording the nodes in the JSTypeFeedbackTable, because the overhead should be negligible. Just wanted to check.
https://codereview.chromium.org/1149133005/diff/100001/src/compiler/ast-graph... File src/compiler/ast-graph-builder.cc (right): https://codereview.chromium.org/1149133005/diff/100001/src/compiler/ast-graph... src/compiler/ast-graph-builder.cc:2313: if (!property->IsSuperAccess()) { On 2015/06/05 13:28:19, Michael Starzinger wrote: > Could this be modeled as a switch over the LhsKind as well? If so, can we leave > a TODO here in that regard? I tried it but reverted back to the ifs. I found the switch cleaner but it does mean more code duplication. https://codereview.chromium.org/1149133005/diff/100001/src/compiler/ast-graph... src/compiler/ast-graph-builder.cc:3461: return Record(js_type_feedback_, value, feedback.slot()); On 2015/06/05 13:28:19, Michael Starzinger wrote: > Just for clarification: My understanding is that we don't gather type-feedback > for any of these super loads and stores yet. Correct? I am still fine with > recording the nodes in the JSTypeFeedbackTable, because the overhead should be > negligible. Just wanted to check. I think that is right. At some point we probably want this, at least for loads, I think we can skip it for super-store since it is not nearly as common as super-load.
https://codereview.chromium.org/1149133005/diff/100001/src/compiler/ast-graph... File src/compiler/ast-graph-builder.cc (right): https://codereview.chromium.org/1149133005/diff/100001/src/compiler/ast-graph... src/compiler/ast-graph-builder.cc:2313: if (!property->IsSuperAccess()) { On 2015/06/05 13:50:26, arv wrote: > On 2015/06/05 13:28:19, Michael Starzinger wrote: > > Could this be modeled as a switch over the LhsKind as well? If so, can we > leave > > a TODO here in that regard? > > I tried it but reverted back to the ifs. I found the switch cleaner but it does > mean more code duplication. Acknowledged. OK, then let's leave it as it is. https://codereview.chromium.org/1149133005/diff/100001/src/compiler/ast-graph... src/compiler/ast-graph-builder.cc:3461: return Record(js_type_feedback_, value, feedback.slot()); On 2015/06/05 13:50:26, arv wrote: > On 2015/06/05 13:28:19, Michael Starzinger wrote: > > Just for clarification: My understanding is that we don't gather type-feedback > > for any of these super loads and stores yet. Correct? I am still fine with > > recording the nodes in the JSTypeFeedbackTable, because the overhead should be > > negligible. Just wanted to check. > > I think that is right. At some point we probably want this, at least for loads, > I think we can skip it for super-store since it is not nearly as common as > super-load. Acknowledged.
The CQ bit was checked by arv@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1149133005/100001
Message was sent while issue was closed.
Committed patchset #1 (id:100001)
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/06c706d84c04fc2e31e993518df9dcefb7543a2d Cr-Commit-Position: refs/heads/master@{#28815}
Message was sent while issue was closed.
brucedawson@chromium.org changed reviewers: + brucedawson@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1149133005/diff/100001/src/compiler/ast-graph... File src/compiler/ast-graph-builder.cc (right): https://codereview.chromium.org/1149133005/diff/100001/src/compiler/ast-graph... src/compiler/ast-graph-builder.cc:2219: Node* value; It is not clear that 'value' will always be initialized before it is used by this function. This will be true as long as GetAssignType() always returns one of the five values listed in the switch statement and this may not be a long-term stable assumption. Consider initializing value to nullptr when it is declared, thus making it trivially obvious that it is always initialized. This is virtually free. https://codereview.chromium.org/1149133005/diff/100001/src/compiler/ast-graph... src/compiler/ast-graph-builder.cc:2225: value = nullptr; i.e.; move this assignment to the variable declaration. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
