Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2684973002:
[ic] Inline LoadGlobalIC in bytecode handlers (Closed)
Description[ic] Inline LoadGlobalIC in bytecode handlers
Instead of calling the LoadGlobalIC stub, bytecode handlers now inline
logic for LoadGlobalIC. The LoadGlobalICData case takes a fast path
which omits name loading and frame construction.
BUG=v8:5917
Review-Url: https://codereview.chromium.org/2684973002
Cr-Commit-Position: refs/heads/master@{#43210}
Committed: https://chromium.googlesource.com/v8/v8/+/63096bc89d674d6c97bdecdbdfbf98871d1825e0
Patch Set 1 #Patch Set 2 : Compile fix for windows #Patch Set 3 : Remove gen_context arg #
Total comments: 16
Patch Set 4 : Rebase and address comments #
Total comments: 4
Patch Set 5 : Address comments #Messages
Total messages: 35 (23 generated)
The CQ bit was checked by jgruber@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_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/22250) v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/3...)
The CQ bit was checked by jgruber@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 checked by jgruber@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...
jgruber@chromium.org changed reviewers: + ishell@chromium.org, rmcilroy@chromium.org
Please take a look, this CL does the actual inlining in LdaGlobal & co.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_arm64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng_trigg...)
Overall good. Please rebase. https://codereview.chromium.org/2684973002/diff/40001/src/interpreter/interpr... File src/interpreter/interpreter.cc (right): https://codereview.chromium.org/2684973002/diff/40001/src/interpreter/interpr... src/interpreter/interpreter.cc:463: const std::function<compiler::Node*()>& gen_name_index, Node* feedback_slot, Given that you already sawed LoadGlobalIC into building blocks I think you can just pass the name_operand_index and slot_operand_index to this function and load them here. Ross, WDYT? https://codereview.chromium.org/2684973002/diff/40001/src/interpreter/interpr... src/interpreter/interpreter.cc:482: accessor_asm.LoadGlobalICData(&p, &exit_point, &try_handler, &miss); s/&p/feedback_vector, smi_slot/
Out of interest, do you see any perf improvement with this? Do we manage to avoid building a frame for the simple inlined getters in the generated bytecode handler code? https://codereview.chromium.org/2684973002/diff/40001/src/interpreter/interpr... File src/interpreter/interpreter.cc (right): https://codereview.chromium.org/2684973002/diff/40001/src/interpreter/interpr... src/interpreter/interpreter.cc:463: const std::function<compiler::Node*()>& gen_name_index, Node* feedback_slot, On 2017/02/08 15:42:20, Igor Sheludko wrote: > Given that you already sawed LoadGlobalIC into building blocks I think you can > just pass the name_operand_index and slot_operand_index to this function and > load them here. > > Ross, WDYT? +1 I think you should be able to just pass the name_index as a node directly, no?
https://codereview.chromium.org/2684973002/diff/40001/src/interpreter/interpr... File src/interpreter/interpreter.cc (right): https://codereview.chromium.org/2684973002/diff/40001/src/interpreter/interpr... src/interpreter/interpreter.cc:463: const std::function<compiler::Node*()>& gen_name_index, Node* feedback_slot, On 2017/02/08 17:39:14, rmcilroy wrote: > On 2017/02/08 15:42:20, Igor Sheludko wrote: > > Given that you already sawed LoadGlobalIC into building blocks I think you can > > just pass the name_operand_index and slot_operand_index to this function and > > load them here. > > > > Ross, WDYT? > > +1 I think you should be able to just pass the name_index as a node directly, > no? The idea is to remove everything related to name from the fast path where we load a property cell from the vector and check if it contains a hole.
https://codereview.chromium.org/2684973002/diff/40001/src/interpreter/interpr... File src/interpreter/interpreter.cc (right): https://codereview.chromium.org/2684973002/diff/40001/src/interpreter/interpr... src/interpreter/interpreter.cc:463: const std::function<compiler::Node*()>& gen_name_index, Node* feedback_slot, On 2017/02/08 17:44:40, Igor Sheludko wrote: > On 2017/02/08 17:39:14, rmcilroy wrote: > > On 2017/02/08 15:42:20, Igor Sheludko wrote: > > > Given that you already sawed LoadGlobalIC into building blocks I think you > can > > > just pass the name_operand_index and slot_operand_index to this function and > > > load them here. > > > > > > Ross, WDYT? > > > > +1 I think you should be able to just pass the name_index as a node directly, > > no? > > The idea is to remove everything related to name from the fast path where we > load a property cell from the vector and check if it contains a hole. And in my comment above I meant to pass indices as ints.
Looks good other than the generator function and a couple of nits. Thanks. https://codereview.chromium.org/2684973002/diff/40001/src/interpreter/interpr... File src/interpreter/interpreter.cc (right): https://codereview.chromium.org/2684973002/diff/40001/src/interpreter/interpr... src/interpreter/interpreter.cc:463: const std::function<compiler::Node*()>& gen_name_index, Node* feedback_slot, On 2017/02/08 17:46:25, Igor Sheludko wrote: > On 2017/02/08 17:44:40, Igor Sheludko wrote: > > On 2017/02/08 17:39:14, rmcilroy wrote: > > > On 2017/02/08 15:42:20, Igor Sheludko wrote: > > > > Given that you already sawed LoadGlobalIC into building blocks I think you > > can > > > > just pass the name_operand_index and slot_operand_index to this function > and > > > > load them here. > > > > > > > > Ross, WDYT? > > > > > > +1 I think you should be able to just pass the name_index as a node > directly, > > > no? > > > > The idea is to remove everything related to name from the fast path where we > > load a property cell from the vector and check if it contains a hole. > > And in my comment above I meant to pass indices as ints. I see. I could live with the bytecode operand index being passed as an argument. https://codereview.chromium.org/2684973002/diff/40001/src/interpreter/interpr... src/interpreter/interpreter.cc:476: Label out(assembler); nit - "done" is used more commonly. https://codereview.chromium.org/2684973002/diff/40001/src/interpreter/interpr... src/interpreter/interpreter.cc:480: AccessorAssembler::LoadICParameters p(nullptr, nullptr, nullptr, smi_slot, nit - params https://codereview.chromium.org/2684973002/diff/40001/src/interpreter/interpr... src/interpreter/interpreter.cc:491: Label out(assembler); ditto. https://codereview.chromium.org/2684973002/diff/40001/src/interpreter/interpr... src/interpreter/interpreter.cc:500: AccessorAssembler::LoadICParameters p(context, nullptr, name, smi_slot, ditto
On 2017/02/08 17:39:14, rmcilroy wrote: > Out of interest, do you see any perf improvement with this? Do we manage to > avoid building a frame for the simple inlined getters in the generated bytecode > handler code? Yes! The fast path is frame-less and we seem to get up to 11% speedup on sunspider (locally). I'll respond to other comments in a bit once I figured out the arm test failures.
The CQ bit was checked by jgruber@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...
Addressed comments & rebased in new patch set. arm and arm64 will still fail (IC::address can get confused when Runtime::kLoadIC_Miss is called from a handler). Igor is looking into a fix for this and will land it in a separate CL. https://codereview.chromium.org/2684973002/diff/40001/src/interpreter/interpr... File src/interpreter/interpreter.cc (right): https://codereview.chromium.org/2684973002/diff/40001/src/interpreter/interpr... src/interpreter/interpreter.cc:463: const std::function<compiler::Node*()>& gen_name_index, Node* feedback_slot, On 2017/02/08 18:38:21, rmcilroy wrote: > On 2017/02/08 17:46:25, Igor Sheludko wrote: > > On 2017/02/08 17:44:40, Igor Sheludko wrote: > > > On 2017/02/08 17:39:14, rmcilroy wrote: > > > > On 2017/02/08 15:42:20, Igor Sheludko wrote: > > > > > Given that you already sawed LoadGlobalIC into building blocks I think > you > > > can > > > > > just pass the name_operand_index and slot_operand_index to this function > > and > > > > > load them here. > > > > > > > > > > Ross, WDYT? > > > > > > > > +1 I think you should be able to just pass the name_index as a node > > directly, > > > > no? > > > > > > The idea is to remove everything related to name from the fast path where we > > > load a property cell from the vector and check if it contains a hole. > > > > And in my comment above I meant to pass indices as ints. > > I see. I could live with the bytecode operand index being passed as an argument. This is what I went with, BuildLoadGlobal now takes an integer name_index_index and slot_index and loads the parameters as needed. https://codereview.chromium.org/2684973002/diff/40001/src/interpreter/interpr... src/interpreter/interpreter.cc:476: Label out(assembler); On 2017/02/08 18:38:21, rmcilroy wrote: > nit - "done" is used more commonly. Done. https://codereview.chromium.org/2684973002/diff/40001/src/interpreter/interpr... src/interpreter/interpreter.cc:480: AccessorAssembler::LoadICParameters p(nullptr, nullptr, nullptr, smi_slot, On 2017/02/08 18:38:21, rmcilroy wrote: > nit - params These have been removed. https://codereview.chromium.org/2684973002/diff/40001/src/interpreter/interpr... src/interpreter/interpreter.cc:482: accessor_asm.LoadGlobalICData(&p, &exit_point, &try_handler, &miss); On 2017/02/08 15:42:20, Igor Sheludko wrote: > s/&p/feedback_vector, smi_slot/ Done. Also removed smi tagging for the fast path. https://codereview.chromium.org/2684973002/diff/40001/src/interpreter/interpr... src/interpreter/interpreter.cc:491: Label out(assembler); On 2017/02/08 18:38:21, rmcilroy wrote: > ditto. Done. https://codereview.chromium.org/2684973002/diff/40001/src/interpreter/interpr... src/interpreter/interpreter.cc:500: AccessorAssembler::LoadICParameters p(context, nullptr, name, smi_slot, On 2017/02/08 18:38:21, rmcilroy wrote: > ditto Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_arm64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng_trigg...)
> Yes! The fast path is frame-less and we seem to get up to 11% speedup on ? sunspider (locally). Nice! :). LGTM once final comments are addressed. https://codereview.chromium.org/2684973002/diff/60001/src/interpreter/interpr... File src/interpreter/interpreter.cc (right): https://codereview.chromium.org/2684973002/diff/60001/src/interpreter/interpr... src/interpreter/interpreter.cc:462: void Interpreter::BuildLoadGlobal(int slot_index, int name_index_index, name_index_index? ;) How about name_operand_index? (also rename slot_index to slot_operand_index https://codereview.chromium.org/2684973002/diff/60001/src/interpreter/interpr... File src/interpreter/interpreter.h (right): https://codereview.chromium.org/2684973002/diff/60001/src/interpreter/interpr... src/interpreter/interpreter.h:8: #include <functional> no longer needed?
The CQ bit was checked by jgruber@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/2684973002/diff/60001/src/interpreter/interpr... File src/interpreter/interpreter.cc (right): https://codereview.chromium.org/2684973002/diff/60001/src/interpreter/interpr... src/interpreter/interpreter.cc:462: void Interpreter::BuildLoadGlobal(int slot_index, int name_index_index, On 2017/02/09 10:30:44, rmcilroy wrote: > name_index_index? ;) How about name_operand_index? (also rename slot_index to > slot_operand_index I liked name_index_index because it implies that the operand at that index is not actually the name but its index. But acknowledged that it reads awkwardly :) Done. https://codereview.chromium.org/2684973002/diff/60001/src/interpreter/interpr... File src/interpreter/interpreter.h (right): https://codereview.chromium.org/2684973002/diff/60001/src/interpreter/interpr... src/interpreter/interpreter.h:8: #include <functional> On 2017/02/09 10:30:45, rmcilroy wrote: > no longer needed? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_arm64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng_trigg...)
The CQ bit was checked by jgruber@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rmcilroy@chromium.org Link to the patchset: https://codereview.chromium.org/2684973002/#ps80001 (title: "Address comments")
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": 80001, "attempt_start_ts": 1487163940405750,
"parent_rev": "fdc78d294e79aa920d4d559d672da8ab8bbb2df8", "commit_rev":
"63096bc89d674d6c97bdecdbdfbf98871d1825e0"}
Message was sent while issue was closed.
Description was changed from ========== [ic] Inline LoadGlobalIC in bytecode handlers Instead of calling the LoadGlobalIC stub, bytecode handlers now inline logic for LoadGlobalIC. The LoadGlobalICData case takes a fast path which omits name loading and frame construction. BUG=v8:5917 ========== to ========== [ic] Inline LoadGlobalIC in bytecode handlers Instead of calling the LoadGlobalIC stub, bytecode handlers now inline logic for LoadGlobalIC. The LoadGlobalICData case takes a fast path which omits name loading and frame construction. BUG=v8:5917 Review-Url: https://codereview.chromium.org/2684973002 Cr-Commit-Position: refs/heads/master@{#43210} Committed: https://chromium.googlesource.com/v8/v8/+/63096bc89d674d6c97bdecdbdfbf98871d1... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/v8/v8/+/63096bc89d674d6c97bdecdbdfbf98871d1... |
