Commit b21cda74 authored by Leszek Swirski's avatar Leszek Swirski Committed by Commit Bot

Reland "[ic] Load name/context lazily in LdaNamedProperty"

This reverts commit 5377e72c.

Reason for revert: Looks like the relevant graphs didn't recover after
this revert, which suggests that the regression was an unrelated
secondary effect. Relanding the original change since the revert did
cause some microbenchmark regressions.

Original change's description:
> Revert "[ic] Load name/context lazily in LdaNamedProperty"
> 
> This reverts commit 347092ac.
> 
> Not a clean revert, since other changes got baked on top, but rather
> a manual removal of LoadLazyICParameters.
> 
> Reason for revert: Seems to actually regress bindings perf tests (see
> bugs and https://chromeperf.appspot.com/group_report?rev=62539), doesn't
> seem to improve performance elsewhere, and increases complexity.
> 
> Original change's description:
> > [ic] Load name/context lazily in LdaNamedProperty
> >
> > Introduces LazyLoadICParameters which allow a LazyNode for context and
> > name. These aren't used on the fast path, so we want to avoid reading
> > them for both performance and register pressure reasons.
> >
> > Change-Id: Ifb637cf4782ce984feee9af503998e7539beb823
> > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1686665
> > Commit-Queue: Leszek Swirski <leszeks@chromium.org>
> > Reviewed-by: Toon Verwaest <verwaest@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#62539}
> 
> # Not skipping CQ checks because original CL landed > 1 day ago.
> 
> Bug: chromium:981797
> Bug: chromium:982630
> Change-Id: I88af764d17afb76d6e64b95a3d1e4aaa1c6c8978
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1934327
> Auto-Submit: Leszek Swirski <leszeks@chromium.org>
> Reviewed-by: Toon Verwaest <verwaest@chromium.org>
> Commit-Queue: Toon Verwaest <verwaest@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#65205}

TBR=leszeks@chromium.org,verwaest@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: chromium:981797, chromium:982630, v8:10059
Change-Id: I13754de06c83439e03e22cfaa7a14ce454076db9
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1973730Reviewed-by: 's avatarLeszek Swirski <leszeks@chromium.org>
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#65499}
parent eae14b55
This diff is collapsed.
......@@ -105,14 +105,54 @@ class V8_EXPORT_PRIVATE AccessorAssembler : public CodeStubAssembler {
base::Optional<TNode<Object>> holder_;
};
struct LazyLoadICParameters {
LazyLoadICParameters(LazyNode<Context> context, TNode<Object> receiver,
LazyNode<Object> name, LazyNode<Smi> slot,
TNode<HeapObject> vector,
base::Optional<TNode<Object>> holder = base::nullopt)
: context_(context),
receiver_(receiver),
name_(name),
slot_(slot),
vector_(vector),
holder_(holder ? holder.value() : receiver) {}
explicit LazyLoadICParameters(const LoadICParameters* p)
: receiver_(p->receiver()),
vector_(p->vector()),
holder_(p->holder()) {
slot_ = [=] { return p->slot(); };
context_ = [=] { return p->context(); };
name_ = [=] { return p->name(); };
}
TNode<Context> context() const { return context_(); }
TNode<Object> receiver() const { return receiver_; }
TNode<Object> name() const { return name_(); }
TNode<Smi> slot() const { return slot_(); }
TNode<HeapObject> vector() const { return vector_; }
TNode<Object> holder() const { return holder_; }
private:
LazyNode<Context> context_;
TNode<Object> receiver_;
LazyNode<Object> name_;
LazyNode<Smi> slot_;
TNode<HeapObject> vector_;
TNode<Object> holder_;
};
void LoadGlobalIC(TNode<HeapObject> maybe_feedback_vector,
TNode<Smi> smi_slot, TNode<UintPtrT> slot,
TNode<Context> context, TNode<Name> name,
TypeofMode typeof_mode, ExitPoint* exit_point);
const LazyNode<Smi>& lazy_smi_slot,
const LazyNode<UintPtrT>& lazy_slot,
const LazyNode<Context>& lazy_context,
const LazyNode<Name>& lazy_name, TypeofMode typeof_mode,
ExitPoint* exit_point);
// Specialized LoadIC for inlined bytecode handler, hand-tuned to omit frame
// construction on common paths.
void LoadIC_BytecodeHandler(const LoadICParameters* p, ExitPoint* exit_point);
void LoadIC_BytecodeHandler(const LazyLoadICParameters* p,
ExitPoint* exit_point);
// Loads dataX field from the DataHandler object.
TNode<MaybeObject> LoadHandlerDataField(TNode<DataHandler> handler,
......@@ -213,13 +253,13 @@ class V8_EXPORT_PRIVATE AccessorAssembler : public CodeStubAssembler {
// LoadIC implementation.
void HandleLoadICHandlerCase(
const LoadICParameters* p, TNode<Object> handler, Label* miss,
const LazyLoadICParameters* p, TNode<Object> handler, Label* miss,
ExitPoint* exit_point, ICMode ic_mode = ICMode::kNonGlobalIC,
OnNonExistent on_nonexistent = OnNonExistent::kReturnUndefined,
ElementSupport support_elements = kOnlyProperties,
LoadAccessMode access_mode = LoadAccessMode::kLoad);
void HandleLoadICSmiHandlerCase(const LoadICParameters* p,
void HandleLoadICSmiHandlerCase(const LazyLoadICParameters* p,
TNode<Object> holder, TNode<Smi> smi_handler,
TNode<Object> handler, Label* miss,
ExitPoint* exit_point, ICMode ic_mode,
......@@ -227,7 +267,7 @@ class V8_EXPORT_PRIVATE AccessorAssembler : public CodeStubAssembler {
ElementSupport support_elements,
LoadAccessMode access_mode);
void HandleLoadICProtoHandler(const LoadICParameters* p,
void HandleLoadICProtoHandler(const LazyLoadICParameters* p,
TNode<DataHandler> handler,
TVariable<Object>* var_holder,
TVariable<Object>* var_smi_handler,
......@@ -235,12 +275,12 @@ class V8_EXPORT_PRIVATE AccessorAssembler : public CodeStubAssembler {
ExitPoint* exit_point, ICMode ic_mode,
LoadAccessMode access_mode);
void HandleLoadCallbackProperty(const LoadICParameters* p,
void HandleLoadCallbackProperty(const LazyLoadICParameters* p,
TNode<JSObject> holder,
TNode<WordT> handler_word,
ExitPoint* exit_point);
void HandleLoadAccessor(const LoadICParameters* p,
void HandleLoadAccessor(const LazyLoadICParameters* p,
TNode<CallHandlerInfo> call_handler_info,
TNode<WordT> handler_word, TNode<DataHandler> handler,
TNode<IntPtrT> handler_kind, ExitPoint* exit_point);
......@@ -254,13 +294,13 @@ class V8_EXPORT_PRIVATE AccessorAssembler : public CodeStubAssembler {
Label* can_access, Label* miss);
void HandleLoadICSmiHandlerLoadNamedCase(
const LoadICParameters* p, TNode<Object> holder,
const LazyLoadICParameters* p, TNode<Object> holder,
TNode<IntPtrT> handler_kind, TNode<WordT> handler_word,
Label* rebox_double, TVariable<Float64T>* var_double_value,
TNode<Object> handler, Label* miss, ExitPoint* exit_point, ICMode ic_mode,
OnNonExistent on_nonexistent, ElementSupport support_elements);
void HandleLoadICSmiHandlerHasNamedCase(const LoadICParameters* p,
void HandleLoadICSmiHandlerHasNamedCase(const LazyLoadICParameters* p,
TNode<Object> holder,
TNode<IntPtrT> handler_kind,
Label* miss, ExitPoint* exit_point,
......@@ -270,13 +310,15 @@ class V8_EXPORT_PRIVATE AccessorAssembler : public CodeStubAssembler {
void LoadGlobalIC_TryPropertyCellCase(TNode<FeedbackVector> vector,
TNode<UintPtrT> slot,
TNode<Context> context,
const LazyNode<Context>& lazy_context,
ExitPoint* exit_point,
Label* try_handler, Label* miss);
void LoadGlobalIC_TryHandlerCase(TNode<FeedbackVector> vector,
TNode<UintPtrT> slot, TNode<Smi> smi_slot,
TNode<Context> context, TNode<Name> name,
TNode<UintPtrT> slot,
const LazyNode<Smi>& lazy_smi_slot,
const LazyNode<Context>& lazy_context,
const LazyNode<Name>& lazy_name,
TypeofMode typeof_mode,
ExitPoint* exit_point, Label* miss);
......
......@@ -167,15 +167,25 @@ class InterpreterLoadGlobalAssembler : public InterpreterAssembler {
Dispatch();
});
TNode<Smi> smi_slot =
SmiTag(Signed(BytecodeOperandIdx(slot_operand_index)));
TNode<UintPtrT> slot = BytecodeOperandIdx(slot_operand_index);
TNode<Context> context = GetContext();
TNode<Name> name =
CAST(LoadConstantPoolEntryAtOperandIndex(name_operand_index));
LazyNode<Smi> lazy_smi_slot = [=] {
return SmiTag(Signed(BytecodeOperandIdx(slot_operand_index)));
};
LazyNode<UintPtrT> lazy_slot = [=] {
return BytecodeOperandIdx(slot_operand_index);
};
LazyNode<Context> lazy_context = [=] { return GetContext(); };
accessor_asm.LoadGlobalIC(maybe_feedback_vector, smi_slot, slot, context,
name, typeof_mode, &exit_point);
LazyNode<Name> lazy_name = [=] {
TNode<Name> name =
CAST(LoadConstantPoolEntryAtOperandIndex(name_operand_index));
return name;
};
accessor_asm.LoadGlobalIC(maybe_feedback_vector, lazy_smi_slot, lazy_slot,
lazy_context, lazy_name, typeof_mode,
&exit_point);
}
};
......@@ -509,16 +519,18 @@ IGNITION_HANDLER(LdaNamedProperty, InterpreterAssembler) {
TNode<Object> recv = LoadRegisterAtOperandIndex(0);
// Load the name and context lazily.
TNode<Smi> smi_slot = SmiTag(Signed(feedback_slot));
TNode<Name> name = CAST(LoadConstantPoolEntryAtOperandIndex(1));
TNode<Context> context = GetContext();
LazyNode<Smi> lazy_smi_slot = [=] { return SmiTag(Signed(feedback_slot)); };
LazyNode<Name> lazy_name = [=] {
return CAST(LoadConstantPoolEntryAtOperandIndex(1));
};
LazyNode<Context> lazy_context = [=] { return GetContext(); };
Label done(this);
TVARIABLE(Object, var_result);
ExitPoint exit_point(this, &done, &var_result);
AccessorAssembler::LoadICParameters params(context, recv, name, smi_slot,
feedback_vector);
AccessorAssembler::LazyLoadICParameters params(
lazy_context, recv, lazy_name, lazy_smi_slot, feedback_vector);
AccessorAssembler accessor_asm(state());
accessor_asm.LoadIC_BytecodeHandler(&params, &exit_point);
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment