Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2559173003:
[compiler] Generalize JSContextSpecialization. (Closed)
|
Created:
4 years ago by neis Modified:
3 years, 11 months ago Reviewers:
Michael Starzinger CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[compiler] Generalize JSContextSpecialization.
With this CL, context loads and stores are "strengthened" by reducing
the incoming context chain and decreasing the depth accordingly,
whenever possible. This enables more opportunities for specialization
and will let us easily add module context specialization later.
BUG=
Review-Url: https://codereview.chromium.org/2559173003
Cr-Commit-Position: refs/heads/master@{#42334}
Committed: https://chromium.googlesource.com/v8/v8/+/fd8cebb1a6676dcb4311b660678b75c1d24e5744
Patch Set 1 : . #
Total comments: 6
Patch Set 2 : Feedback #
Messages
Total messages: 35 (30 generated)
The CQ bit was checked by neis@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
The CQ bit was checked by neis@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== wip COMMIT=false BUG= ========== to ========== [compiler] Generalize JSContextSpecialization. With this CL, context loads and stores are "strengthened" by reducing the incoming context chain and decrementing the depth accordingly, whenever possible. This enables more opportunities for specialization and will be particularly useful when we support module context specialization later. BUG= ==========
neis@chromium.org changed reviewers: + mstarzinger@chromium.org
Patchset #1 (id:1) has been deleted
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
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
The CQ bit was checked by neis@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [compiler] Generalize JSContextSpecialization. With this CL, context loads and stores are "strengthened" by reducing the incoming context chain and decrementing the depth accordingly, whenever possible. This enables more opportunities for specialization and will be particularly useful when we support module context specialization later. BUG= ========== to ========== [compiler] Generalize JSContextSpecialization. With this CL, context loads and stores are "strengthened" by reducing the incoming context chain and decreasing the depth accordingly, whenever possible. This enables more opportunities for specialization and will be particularly useful when we support module context specialization later. BUG= ==========
Description was changed from ========== [compiler] Generalize JSContextSpecialization. With this CL, context loads and stores are "strengthened" by reducing the incoming context chain and decreasing the depth accordingly, whenever possible. This enables more opportunities for specialization and will be particularly useful when we support module context specialization later. BUG= ========== to ========== [compiler] Generalize JSContextSpecialization. With this CL, context loads and stores are "strengthened" by reducing the incoming context chain and decreasing the depth accordingly, whenever possible. This enables more opportunities for specialization and will let us easily add module context specialization later. BUG= ==========
PTAL and let me know what you think.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM, cool stuff, only nits! https://codereview.chromium.org/2559173003/diff/160001/src/compiler/js-contex... File src/compiler/js-context-specialization.cc (right): https://codereview.chromium.org/2559173003/diff/160001/src/compiler/js-contex... src/compiler/js-context-specialization.cc:40: } // XXX: Move to node-properties? nit: s/XXX/TODO/ here. Also maybe move to in front of the function. https://codereview.chromium.org/2559173003/diff/160001/src/compiler/js-contex... src/compiler/js-context-specialization.cc:44: Reduction JSContextSpecialization::StrengthenJSLoadContext(Node* node, random non-actionable rambling: As discussed offline, the name "strengthen" confused me at first. In my interpretation of terminology this performs a "reduction of strength". My interpretation might be completely wrong though. https://codereview.chromium.org/2559173003/diff/160001/src/compiler/opcodes.h File src/compiler/opcodes.h (right): https://codereview.chromium.org/2559173003/diff/160001/src/compiler/opcodes.h... src/compiler/opcodes.h:799: static bool IsContextExtendingOpcode(Value value) { nit: Can we either rename this predicate to "IsContextChainExtendingOpcode" or at least add a comment that explains this is about extending the context _chain_. Otherwise this risks being confused with the context extension we perform for e.h. with-contexts. A comment might be useful either way.
The CQ bit was checked by neis@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2559173003/diff/160001/src/compiler/js-contex... File src/compiler/js-context-specialization.cc (right): https://codereview.chromium.org/2559173003/diff/160001/src/compiler/js-contex... src/compiler/js-context-specialization.cc:40: } // XXX: Move to node-properties? On 2017/01/13 13:36:26, Michael Starzinger wrote: > nit: s/XXX/TODO/ here. Also maybe move to in front of the function. I actually moved it to NodeProperties now. https://codereview.chromium.org/2559173003/diff/160001/src/compiler/js-contex... src/compiler/js-context-specialization.cc:44: Reduction JSContextSpecialization::StrengthenJSLoadContext(Node* node, On 2017/01/13 13:36:26, Michael Starzinger wrote: > random non-actionable rambling: As discussed offline, the name "strengthen" > confused me at first. In my interpretation of terminology this performs a > "reduction of strength". My interpretation might be completely wrong though. Renamed to SimplifyJS... https://codereview.chromium.org/2559173003/diff/160001/src/compiler/opcodes.h File src/compiler/opcodes.h (right): https://codereview.chromium.org/2559173003/diff/160001/src/compiler/opcodes.h... src/compiler/opcodes.h:799: static bool IsContextExtendingOpcode(Value value) { On 2017/01/13 13:36:26, Michael Starzinger wrote: > nit: Can we either rename this predicate to "IsContextChainExtendingOpcode" or > at least add a comment that explains this is about extending the context > _chain_. Otherwise this risks being confused with the context extension we > perform for e.h. with-contexts. A comment might be useful either way. Done. Totally agree.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by neis@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mstarzinger@chromium.org Link to the patchset: https://codereview.chromium.org/2559173003/#ps180001 (title: "Feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1484318572636570,
"parent_rev": "a6fe748d5346a39994c80bc99bbfb5526a9a0319", "commit_rev":
"fd8cebb1a6676dcb4311b660678b75c1d24e5744"}
Message was sent while issue was closed.
Description was changed from ========== [compiler] Generalize JSContextSpecialization. With this CL, context loads and stores are "strengthened" by reducing the incoming context chain and decreasing the depth accordingly, whenever possible. This enables more opportunities for specialization and will let us easily add module context specialization later. BUG= ========== to ========== [compiler] Generalize JSContextSpecialization. With this CL, context loads and stores are "strengthened" by reducing the incoming context chain and decreasing the depth accordingly, whenever possible. This enables more opportunities for specialization and will let us easily add module context specialization later. BUG= Review-Url: https://codereview.chromium.org/2559173003 Cr-Commit-Position: refs/heads/master@{#42334} Committed: https://chromium.googlesource.com/v8/v8/+/fd8cebb1a6676dcb4311b660678b75c1d24... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:180001) as https://chromium.googlesource.com/v8/v8/+/fd8cebb1a6676dcb4311b660678b75c1d24... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
