Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2688503002:
[ic] Refactor LoadGlobalIC in preparation for handler inlining (Closed)
|
Created:
3 years, 10 months ago by jgruber Modified:
3 years, 10 months ago Reviewers:
Igor Sheludko CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[ic] Refactor LoadGlobalIC in preparation for handler inlining
LoadGlobalIC will be inlined into ignition's LdaGlobal family of bytecode
handlers. This CL splits up LoadGlobalIC into three distinct cases (property
cell, handler, and miss) and introduces the ExitPoint abstraction in order
to make inlining easier.
BUG=v8:5917
Review-Url: https://codereview.chromium.org/2688503002
Cr-Commit-Position: refs/heads/master@{#43055}
Committed: https://chromium.googlesource.com/v8/v8/+/f46f341303ac4efad5cbcc771d10642aa2c826de
Patch Set 1 #
Total comments: 18
Patch Set 2 : Address comments #
Dependent Patchsets: Messages
Total messages: 21 (16 generated)
The CQ bit was checked by jgruber@chromium.org to run a CQ dry run
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
Hey Igor, this is the initial refactoring in preparation for inlining. It splits up LoadGlobalIC (and exposes these helper functions) and adds the ExitPoint mechanic. There shouldn't be any behavioral changes in this CL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with nits: https://codereview.chromium.org/2688503002/diff/1/src/ic/accessor-assembler.cc File src/ic/accessor-assembler.cc (right): https://codereview.chromium.org/2688503002/diff/1/src/ic/accessor-assembler.c... src/ic/accessor-assembler.cc:1488: void AccessorAssembler::LoadGlobalICData(const LoadICParameters* p, LoadGlobalIC_TryPropertyCellCase(Node* vector, Node* slot, ... to make it explicit that the receiver, context and name are not required here. https://codereview.chromium.org/2688503002/diff/1/src/ic/accessor-assembler.c... src/ic/accessor-assembler.cc:1491: Comment("LoadGlobalICData"); Same here. https://codereview.chromium.org/2688503002/diff/1/src/ic/accessor-assembler.c... src/ic/accessor-assembler.cc:1506: void AccessorAssembler::LoadGlobalICHandler(const LoadICParameters* p, LoadGlobalIC_TryHandlerCase https://codereview.chromium.org/2688503002/diff/1/src/ic/accessor-assembler.c... src/ic/accessor-assembler.cc:1510: Comment("LoadGlobalICHandler"); Same here. https://codereview.chromium.org/2688503002/diff/1/src/ic/accessor-assembler.h File src/ic/accessor-assembler.h (right): https://codereview.chromium.org/2688503002/diff/1/src/ic/accessor-assembler.h... src/ic/accessor-assembler.h:226: DCHECK(out_ != nullptr || var_result_ == nullptr); DCHECK_EQ(out != nullptr, var_result != nullptr); https://codereview.chromium.org/2688503002/diff/1/src/ic/accessor-assembler.h... src/ic/accessor-assembler.h:230: void ReturnRuntime(Runtime::FunctionId function, Node* context, ReturnCallRuntime https://codereview.chromium.org/2688503002/diff/1/src/ic/accessor-assembler.h... src/ic/accessor-assembler.h:240: void ReturnStub(Callable const& callable, Node* context, TArgs... args) { ReturnCallStub https://codereview.chromium.org/2688503002/diff/1/src/ic/accessor-assembler.h... src/ic/accessor-assembler.h:249: void ReturnStub(const CallInterfaceDescriptor& descriptor, Node* target, ReturnCallStub https://codereview.chromium.org/2688503002/diff/1/src/ic/accessor-assembler.h... src/ic/accessor-assembler.h:267: bool IsIndirect() const { return !IsDirect(); } I'd just leave IsDirect()
Description was changed from ========== [ic] Refactor LoadGlobalIC in preparation for handler inlining LoadGlobalIC will be inlined into ignition's LdaGlobal family of bytecode handlers. This CL splits up LoadGlobalIC into three distinct cases (data, handler, and miss) and introduces the ExitPoint abstraction in order to make inlining easier. BUG=v8:5917 ========== to ========== [ic] Refactor LoadGlobalIC in preparation for handler inlining LoadGlobalIC will be inlined into ignition's LdaGlobal family of bytecode handlers. This CL splits up LoadGlobalIC into three distinct cases (property cell, handler, and miss) and introduces the ExitPoint abstraction in order to make inlining easier. BUG=v8:5917 ==========
Description was changed from ========== [ic] Refactor LoadGlobalIC in preparation for handler inlining LoadGlobalIC will be inlined into ignition's LdaGlobal family of bytecode handlers. This CL splits up LoadGlobalIC into three distinct cases (property cell, handler, and miss) and introduces the ExitPoint abstraction in order to make inlining easier. BUG=v8:5917 ========== to ========== [ic] Refactor LoadGlobalIC in preparation for handler inlining LoadGlobalIC will be inlined into ignition's LdaGlobal family of bytecode handlers. This CL splits up LoadGlobalIC into three distinct cases (property cell, handler, and miss) and introduces the ExitPoint abstraction in order to make inlining easier. BUG=v8:5917 ==========
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/2688503002/diff/1/src/ic/accessor-assembler.cc File src/ic/accessor-assembler.cc (right): https://codereview.chromium.org/2688503002/diff/1/src/ic/accessor-assembler.c... src/ic/accessor-assembler.cc:1488: void AccessorAssembler::LoadGlobalICData(const LoadICParameters* p, On 2017/02/08 15:26:31, Igor Sheludko wrote: > LoadGlobalIC_TryPropertyCellCase(Node* vector, Node* slot, > > ... to make it explicit that the receiver, context and name are not required > here. SG. I had it like this in an upcoming CL to omit unneeded smi tagging & untagging, so I guess I'll sneak this in here as well with an additional ParameterMode. https://codereview.chromium.org/2688503002/diff/1/src/ic/accessor-assembler.c... src/ic/accessor-assembler.cc:1491: Comment("LoadGlobalICData"); On 2017/02/08 15:26:31, Igor Sheludko wrote: > Same here. Done. https://codereview.chromium.org/2688503002/diff/1/src/ic/accessor-assembler.c... src/ic/accessor-assembler.cc:1506: void AccessorAssembler::LoadGlobalICHandler(const LoadICParameters* p, On 2017/02/08 15:26:31, Igor Sheludko wrote: > LoadGlobalIC_TryHandlerCase Done. https://codereview.chromium.org/2688503002/diff/1/src/ic/accessor-assembler.c... src/ic/accessor-assembler.cc:1510: Comment("LoadGlobalICHandler"); On 2017/02/08 15:26:31, Igor Sheludko wrote: > Same here. Done. https://codereview.chromium.org/2688503002/diff/1/src/ic/accessor-assembler.h File src/ic/accessor-assembler.h (right): https://codereview.chromium.org/2688503002/diff/1/src/ic/accessor-assembler.h... src/ic/accessor-assembler.h:226: DCHECK(out_ != nullptr || var_result_ == nullptr); On 2017/02/08 15:26:32, Igor Sheludko wrote: > DCHECK_EQ(out != nullptr, var_result != nullptr); Done. https://codereview.chromium.org/2688503002/diff/1/src/ic/accessor-assembler.h... src/ic/accessor-assembler.h:230: void ReturnRuntime(Runtime::FunctionId function, Node* context, On 2017/02/08 15:26:32, Igor Sheludko wrote: > ReturnCallRuntime Done. https://codereview.chromium.org/2688503002/diff/1/src/ic/accessor-assembler.h... src/ic/accessor-assembler.h:240: void ReturnStub(Callable const& callable, Node* context, TArgs... args) { On 2017/02/08 15:26:32, Igor Sheludko wrote: > ReturnCallStub Done. https://codereview.chromium.org/2688503002/diff/1/src/ic/accessor-assembler.h... src/ic/accessor-assembler.h:249: void ReturnStub(const CallInterfaceDescriptor& descriptor, Node* target, On 2017/02/08 15:26:32, Igor Sheludko wrote: > ReturnCallStub Done. https://codereview.chromium.org/2688503002/diff/1/src/ic/accessor-assembler.h... src/ic/accessor-assembler.h:267: bool IsIndirect() const { return !IsDirect(); } On 2017/02/08 15:26:32, Igor Sheludko wrote: > I'd just leave IsDirect() Done.
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 jgruber@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org Link to the patchset: https://codereview.chromium.org/2688503002/#ps20001 (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": 20001, "attempt_start_ts": 1486634428511120,
"parent_rev": "e425079b1d91a9927a638e9ca7f7fd079773b4f8", "commit_rev":
"f46f341303ac4efad5cbcc771d10642aa2c826de"}
Message was sent while issue was closed.
Description was changed from ========== [ic] Refactor LoadGlobalIC in preparation for handler inlining LoadGlobalIC will be inlined into ignition's LdaGlobal family of bytecode handlers. This CL splits up LoadGlobalIC into three distinct cases (property cell, handler, and miss) and introduces the ExitPoint abstraction in order to make inlining easier. BUG=v8:5917 ========== to ========== [ic] Refactor LoadGlobalIC in preparation for handler inlining LoadGlobalIC will be inlined into ignition's LdaGlobal family of bytecode handlers. This CL splits up LoadGlobalIC into three distinct cases (property cell, handler, and miss) and introduces the ExitPoint abstraction in order to make inlining easier. BUG=v8:5917 Review-Url: https://codereview.chromium.org/2688503002 Cr-Commit-Position: refs/heads/master@{#43055} Committed: https://chromium.googlesource.com/v8/v8/+/f46f341303ac4efad5cbcc771d10642aa2c... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/v8/v8/+/f46f341303ac4efad5cbcc771d10642aa2c... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
